All of lore.kernel.org
 help / color / mirror / Atom feed
* iwlwifi firmware load broken in current -git
@ 2017-09-12 15:48 Jens Axboe
  2017-09-12 16:11 ` Coelho, Luciano
  0 siblings, 1 reply; 31+ messages in thread
From: Jens Axboe @ 2017-09-12 15:48 UTC (permalink / raw)
  To: Johannes Berg, emmanuel.grumbach, Coelho, Luciano
  Cc: linuxwifi, linux-wireless

Hi,

I have no wifi in current git (8fac2f96ab8), it simply fails with:

[    4.363481] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-34.ucode failed with error -2
[    4.363733] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-33.ucode failed with error -2
[    4.363744] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-32.ucode failed with error -2
[    4.368077] iwlwifi 0000:04:00.0: loaded firmware version 31.532993.0 op_mode iwlmvm
[    4.461753] iwlwifi 0000:04:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208
[ ... ]
[    9.510570] iwlwifi 0000:04:00.0: Failed to load firmware chunk!             
[    9.513903] iwlwifi 0000:04:00.0: Could not load the [0] uCode section       
[    9.516201] iwlwifi 0000:04:00.0: Failed to start INIT ucode: -110           
[    9.530842] iwlwifi 0000:04:00.0: Failed to run INIT ucode: -110             

I get the same thing with -27 of the firmware:

[   60.990831] Intel(R) Wireless WiFi driver for Linux                          
[   60.990833] Copyright(c) 2003- 2015 Intel Corporation                        
[   60.991936] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-34.ucode failed with error -2
[   60.991941] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-33.ucode failed with error -2
[   60.991946] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-32.ucode failed with error -2
[   60.991957] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-31.ucode failed with error -2
[   60.991964] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-30.ucode failed with error -2
[   60.991969] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-29.ucode failed with error -2
[   60.991975] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-28.ucode failed with error -2
[   61.029852] iwlwifi 0000:04:00.0: loaded firmware version 27.541033.0 op_mode iwlmvm
[   61.043660] iwlwifi 0000:04:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208
[   66.070135] iwlwifi 0000:04:00.0: Failed to load firmware chunk!             
[   66.070149] iwlwifi 0000:04:00.0: Could not load the [0] uCode section       
[   66.070165] iwlwifi 0000:04:00.0: Failed to start INIT ucode: -110           
[   66.083462] iwlwifi 0000:04:00.0: Failed to run INIT ucode: -110             

Things work fine in 4.13-rc7+, which was the last kernel I ran on my laptop.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-12 15:48 iwlwifi firmware load broken in current -git Jens Axboe
@ 2017-09-12 16:11 ` Coelho, Luciano
  2017-09-12 16:35   ` Jens Axboe
  2017-09-12 16:36   ` Luca Coelho
  0 siblings, 2 replies; 31+ messages in thread
From: Coelho, Luciano @ 2017-09-12 16:11 UTC (permalink / raw)
  To: Grumbach, Emmanuel, axboe, Berg, Johannes; +Cc: linuxwifi, linux-wireless

SGkgSmVucywNCg0KT24gVHVlLCAyMDE3LTA5LTEyIGF0IDA5OjQ4IC0wNjAwLCBKZW5zIEF4Ym9l
IHdyb3RlOg0KPiBIaSwNCj4gDQo+IEkgaGF2ZSBubyB3aWZpIGluIGN1cnJlbnQgZ2l0ICg4ZmFj
MmY5NmFiOCksIGl0IHNpbXBseSBmYWlscyB3aXRoOg0KPiANCj4gWyAgICA0LjM2MzQ4MV0gaXds
d2lmaSAwMDAwOjA0OjAwLjA6IERpcmVjdCBmaXJtd2FyZSBsb2FkIGZvciBpd2x3aWZpLTgwMDBD
LTM0LnVjb2RlIGZhaWxlZCB3aXRoIGVycm9yIC0yDQo+IFsgICAgNC4zNjM3MzNdIGl3bHdpZmkg
MDAwMDowNDowMC4wOiBEaXJlY3QgZmlybXdhcmUgbG9hZCBmb3IgaXdsd2lmaS04MDAwQy0zMy51
Y29kZSBmYWlsZWQgd2l0aCBlcnJvciAtMg0KPiBbICAgIDQuMzYzNzQ0XSBpd2x3aWZpIDAwMDA6
MDQ6MDAuMDogRGlyZWN0IGZpcm13YXJlIGxvYWQgZm9yIGl3bHdpZmktODAwMEMtMzIudWNvZGUg
ZmFpbGVkIHdpdGggZXJyb3IgLTINCj4gWyAgICA0LjM2ODA3N10gaXdsd2lmaSAwMDAwOjA0OjAw
LjA6IGxvYWRlZCBmaXJtd2FyZSB2ZXJzaW9uIDMxLjUzMjk5My4wIG9wX21vZGUgaXdsbXZtDQo+
IFsgICAgNC40NjE3NTNdIGl3bHdpZmkgMDAwMDowNDowMC4wOiBEZXRlY3RlZCBJbnRlbChSKSBE
dWFsIEJhbmQgV2lyZWxlc3MgQUMgODI2MCwgUkVWPTB4MjA4DQo+IFsgLi4uIF0NCj4gWyAgICA5
LjUxMDU3MF0gaXdsd2lmaSAwMDAwOjA0OjAwLjA6IEZhaWxlZCB0byBsb2FkIGZpcm13YXJlIGNo
dW5rISAgICAgICAgICAgICANCj4gWyAgICA5LjUxMzkwM10gaXdsd2lmaSAwMDAwOjA0OjAwLjA6
IENvdWxkIG5vdCBsb2FkIHRoZSBbMF0gdUNvZGUgc2VjdGlvbiAgICAgICANCj4gWyAgICA5LjUx
NjIwMV0gaXdsd2lmaSAwMDAwOjA0OjAwLjA6IEZhaWxlZCB0byBzdGFydCBJTklUIHVjb2RlOiAt
MTEwICAgICAgICAgICANCj4gWyAgICA5LjUzMDg0Ml0gaXdsd2lmaSAwMDAwOjA0OjAwLjA6IEZh
aWxlZCB0byBydW4gSU5JVCB1Y29kZTogLTExMCAgICAgICAgICAgICANCj4gDQo+IEkgZ2V0IHRo
ZSBzYW1lIHRoaW5nIHdpdGggLTI3IG9mIHRoZSBmaXJtd2FyZToNCj4gDQo+IFsgICA2MC45OTA4
MzFdIEludGVsKFIpIFdpcmVsZXNzIFdpRmkgZHJpdmVyIGZvciBMaW51eCAgICAgICAgICAgICAg
ICAgICAgICAgICAgDQo+IFsgICA2MC45OTA4MzNdIENvcHlyaWdodChjKSAyMDAzLSAyMDE1IElu
dGVsIENvcnBvcmF0aW9uICAgICAgICAgICAgICAgICAgICAgICAgDQo+IFsgICA2MC45OTE5MzZd
IGl3bHdpZmkgMDAwMDowNDowMC4wOiBEaXJlY3QgZmlybXdhcmUgbG9hZCBmb3IgaXdsd2lmaS04
MDAwQy0zNC51Y29kZSBmYWlsZWQgd2l0aCBlcnJvciAtMg0KPiBbICAgNjAuOTkxOTQxXSBpd2x3
aWZpIDAwMDA6MDQ6MDAuMDogRGlyZWN0IGZpcm13YXJlIGxvYWQgZm9yIGl3bHdpZmktODAwMEMt
MzMudWNvZGUgZmFpbGVkIHdpdGggZXJyb3IgLTINCj4gWyAgIDYwLjk5MTk0Nl0gaXdsd2lmaSAw
MDAwOjA0OjAwLjA6IERpcmVjdCBmaXJtd2FyZSBsb2FkIGZvciBpd2x3aWZpLTgwMDBDLTMyLnVj
b2RlIGZhaWxlZCB3aXRoIGVycm9yIC0yDQo+IFsgICA2MC45OTE5NTddIGl3bHdpZmkgMDAwMDow
NDowMC4wOiBEaXJlY3QgZmlybXdhcmUgbG9hZCBmb3IgaXdsd2lmaS04MDAwQy0zMS51Y29kZSBm
YWlsZWQgd2l0aCBlcnJvciAtMg0KPiBbICAgNjAuOTkxOTY0XSBpd2x3aWZpIDAwMDA6MDQ6MDAu
MDogRGlyZWN0IGZpcm13YXJlIGxvYWQgZm9yIGl3bHdpZmktODAwMEMtMzAudWNvZGUgZmFpbGVk
IHdpdGggZXJyb3IgLTINCj4gWyAgIDYwLjk5MTk2OV0gaXdsd2lmaSAwMDAwOjA0OjAwLjA6IERp
cmVjdCBmaXJtd2FyZSBsb2FkIGZvciBpd2x3aWZpLTgwMDBDLTI5LnVjb2RlIGZhaWxlZCB3aXRo
IGVycm9yIC0yDQo+IFsgICA2MC45OTE5NzVdIGl3bHdpZmkgMDAwMDowNDowMC4wOiBEaXJlY3Qg
ZmlybXdhcmUgbG9hZCBmb3IgaXdsd2lmaS04MDAwQy0yOC51Y29kZSBmYWlsZWQgd2l0aCBlcnJv
ciAtMg0KPiBbICAgNjEuMDI5ODUyXSBpd2x3aWZpIDAwMDA6MDQ6MDAuMDogbG9hZGVkIGZpcm13
YXJlIHZlcnNpb24gMjcuNTQxMDMzLjAgb3BfbW9kZSBpd2xtdm0NCj4gWyAgIDYxLjA0MzY2MF0g
aXdsd2lmaSAwMDAwOjA0OjAwLjA6IERldGVjdGVkIEludGVsKFIpIER1YWwgQmFuZCBXaXJlbGVz
cyBBQyA4MjYwLCBSRVY9MHgyMDgNCj4gWyAgIDY2LjA3MDEzNV0gaXdsd2lmaSAwMDAwOjA0OjAw
LjA6IEZhaWxlZCB0byBsb2FkIGZpcm13YXJlIGNodW5rISAgICAgICAgICAgICANCj4gWyAgIDY2
LjA3MDE0OV0gaXdsd2lmaSAwMDAwOjA0OjAwLjA6IENvdWxkIG5vdCBsb2FkIHRoZSBbMF0gdUNv
ZGUgc2VjdGlvbiAgICAgICANCj4gWyAgIDY2LjA3MDE2NV0gaXdsd2lmaSAwMDAwOjA0OjAwLjA6
IEZhaWxlZCB0byBzdGFydCBJTklUIHVjb2RlOiAtMTEwICAgICAgICAgICANCj4gWyAgIDY2LjA4
MzQ2Ml0gaXdsd2lmaSAwMDAwOjA0OjAwLjA6IEZhaWxlZCB0byBydW4gSU5JVCB1Y29kZTogLTEx
MCAgICAgICAgICAgICANCj4gDQo+IFRoaW5ncyB3b3JrIGZpbmUgaW4gNC4xMy1yYzcrLCB3aGlj
aCB3YXMgdGhlIGxhc3Qga2VybmVsIEkgcmFuIG9uIG15IGxhcHRvcC4NCg0KVGhpcyBpcyBzdHJh
bmdlLCBMaW51cyBoYXMgYmVlbiBydW5uaW5nIHRoaXMgc2FtZSBjb21iaW5hdGlvbiB3aXRoIC0z
MQ0KKHdpdGggLTI3IHdlIGhhZCBhIHJlZ3Jlc3Npb24sIGJ1dCBpdCB3YXMgZml4ZWQgcmVjZW50
bHkgYW5kIHRoZSBmaXggaXMNCmluIHRoZSBjdXJyZW50IG1hc3RlcikuICBJIGhhdmUgYWxzbyB0
cmllZCB0aGlzIGNvbWJpbmF0aW9uIHdpdGggYm90aA0KLTI3IGFuZCAtMzEgYWZ0ZXIgbXkgZml4
IGFuZCBpdCB3b3JrcyBmaW5lLg0KDQpUaGVyZSBhcmUgb25seSBhIGNvdXBsZSBvZiBvdGhlciBw
YXRjaGVzIHRoYXQgbWF5IGFmZmVjdCBpd2x3aWZpIHNpbmNlDQp0aGUgcHJldmlvdXMgbmV0LW5l
eHQuZ2l0IHB1bGwgcmVxdWVzdC4uLg0KDQpJJ2xsIHRyeSB0aGlzIGNvbWJpbmF0aW9uIG9uIG15
IG1hY2hpbmUgYW5kIHNlZSBpZiBJIGNhbiByZXByb2R1Y2UgdGhlDQpwcm9ibGVtLg0KDQotLQ0K
Q2hlZXJzLA0KTHVjYS4NCg0K

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-12 16:11 ` Coelho, Luciano
@ 2017-09-12 16:35   ` Jens Axboe
  2017-09-12 16:36   ` Luca Coelho
  1 sibling, 0 replies; 31+ messages in thread
From: Jens Axboe @ 2017-09-12 16:35 UTC (permalink / raw)
  To: Coelho, Luciano, Grumbach, Emmanuel, Berg, Johannes
  Cc: linuxwifi, linux-wireless

On 09/12/2017 10:11 AM, Coelho, Luciano wrote:
> Hi Jens,
> 
> On Tue, 2017-09-12 at 09:48 -0600, Jens Axboe wrote:
>> Hi,
>>
>> I have no wifi in current git (8fac2f96ab8), it simply fails with:
>>
>> [    4.363481] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-34.ucode failed with error -2
>> [    4.363733] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-33.ucode failed with error -2
>> [    4.363744] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-32.ucode failed with error -2
>> [    4.368077] iwlwifi 0000:04:00.0: loaded firmware version 31.532993.0 op_mode iwlmvm
>> [    4.461753] iwlwifi 0000:04:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208
>> [ ... ]
>> [    9.510570] iwlwifi 0000:04:00.0: Failed to load firmware chunk!             
>> [    9.513903] iwlwifi 0000:04:00.0: Could not load the [0] uCode section       
>> [    9.516201] iwlwifi 0000:04:00.0: Failed to start INIT ucode: -110           
>> [    9.530842] iwlwifi 0000:04:00.0: Failed to run INIT ucode: -110             
>>
>> I get the same thing with -27 of the firmware:
>>
>> [   60.990831] Intel(R) Wireless WiFi driver for Linux                          
>> [   60.990833] Copyright(c) 2003- 2015 Intel Corporation                        
>> [   60.991936] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-34.ucode failed with error -2
>> [   60.991941] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-33.ucode failed with error -2
>> [   60.991946] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-32.ucode failed with error -2
>> [   60.991957] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-31.ucode failed with error -2
>> [   60.991964] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-30.ucode failed with error -2
>> [   60.991969] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-29.ucode failed with error -2
>> [   60.991975] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-28.ucode failed with error -2
>> [   61.029852] iwlwifi 0000:04:00.0: loaded firmware version 27.541033.0 op_mode iwlmvm
>> [   61.043660] iwlwifi 0000:04:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208
>> [   66.070135] iwlwifi 0000:04:00.0: Failed to load firmware chunk!             
>> [   66.070149] iwlwifi 0000:04:00.0: Could not load the [0] uCode section       
>> [   66.070165] iwlwifi 0000:04:00.0: Failed to start INIT ucode: -110           
>> [   66.083462] iwlwifi 0000:04:00.0: Failed to run INIT ucode: -110             
>>
>> Things work fine in 4.13-rc7+, which was the last kernel I ran on my laptop.
> 
> This is strange, Linus has been running this same combination with -31
> (with -27 we had a regression, but it was fixed recently and the fix is
> in the current master).  I have also tried this combination with both
> -27 and -31 after my fix and it works fine.

It's 100% reproducible, I booted in and out of current -git and 4.13-rc7+
a few times and the latter never works while the former works fine. I
haven't seen this issue before on previous kernels.

> There are only a couple of other patches that may affect iwlwifi since
> the previous net-next.git pull request...
> 
> I'll try this combination on my machine and see if I can reproduce the
> problem.

Let me know if you have something I can try.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-12 16:11 ` Coelho, Luciano
  2017-09-12 16:35   ` Jens Axboe
@ 2017-09-12 16:36   ` Luca Coelho
  2017-09-12 16:59     ` Jens Axboe
  2017-09-12 19:43     ` Jens Axboe
  1 sibling, 2 replies; 31+ messages in thread
From: Luca Coelho @ 2017-09-12 16:36 UTC (permalink / raw)
  To: Grumbach, Emmanuel, axboe, Berg, Johannes; +Cc: linuxwifi, linux-wireless

On Tue, 2017-09-12 at 16:11 +0000, Coelho, Luciano wrote:
> Hi Jens,
> 
> On Tue, 2017-09-12 at 09:48 -0600, Jens Axboe wrote:
> > Hi,
> > 
> > I have no wifi in current git (8fac2f96ab8), it simply fails with:
> > 
> > [    4.363481] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-34.ucode failed with error -2
> > [    4.363733] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-33.ucode failed with error -2
> > [    4.363744] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-32.ucode failed with error -2
> > [    4.368077] iwlwifi 0000:04:00.0: loaded firmware version 31.532993.0 op_mode iwlmvm
> > [    4.461753] iwlwifi 0000:04:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208
> > [ ... ]
> > [    9.510570] iwlwifi 0000:04:00.0: Failed to load firmware chunk!             
> > [    9.513903] iwlwifi 0000:04:00.0: Could not load the [0] uCode section       
> > [    9.516201] iwlwifi 0000:04:00.0: Failed to start INIT ucode: -110           
> > [    9.530842] iwlwifi 0000:04:00.0: Failed to run INIT ucode: -110             
> > 
> > I get the same thing with -27 of the firmware:
> > 
> > [   60.990831] Intel(R) Wireless WiFi driver for Linux                          
> > [   60.990833] Copyright(c) 2003- 2015 Intel Corporation                        
> > [   60.991936] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-34.ucode failed with error -2
> > [   60.991941] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-33.ucode failed with error -2
> > [   60.991946] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-32.ucode failed with error -2
> > [   60.991957] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-31.ucode failed with error -2
> > [   60.991964] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-30.ucode failed with error -2
> > [   60.991969] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-29.ucode failed with error -2
> > [   60.991975] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-28.ucode failed with error -2
> > [   61.029852] iwlwifi 0000:04:00.0: loaded firmware version 27.541033.0 op_mode iwlmvm
> > [   61.043660] iwlwifi 0000:04:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208
> > [   66.070135] iwlwifi 0000:04:00.0: Failed to load firmware chunk!             
> > [   66.070149] iwlwifi 0000:04:00.0: Could not load the [0] uCode section       
> > [   66.070165] iwlwifi 0000:04:00.0: Failed to start INIT ucode: -110           
> > [   66.083462] iwlwifi 0000:04:00.0: Failed to run INIT ucode: -110             
> > 
> > Things work fine in 4.13-rc7+, which was the last kernel I ran on my laptop.
> 
> This is strange, Linus has been running this same combination with -31
> (with -27 we had a regression, but it was fixed recently and the fix is
> in the current master).  I have also tried this combination with both
> -27 and -31 after my fix and it works fine.
> 
> There are only a couple of other patches that may affect iwlwifi since
> the previous net-next.git pull request...
> 
> I'll try this combination on my machine and see if I can reproduce the
> problem.

I just booted my machine with the latest linux.git/master (8fac2f96ab86)
and it works without a problem:

[    8.902174] Intel(R) Wireless WiFi driver for Linux
[    8.902174] Copyright(c) 2003- 2015 Intel Corporation
[    8.995112] iwlwifi 0000:02:00.0: Direct firmware load for iwlwifi-8000C-34.ucode failed with error -2
[    8.995136] iwlwifi 0000:02:00.0: Direct firmware load for iwlwifi-8000C-33.ucode failed with error -2
[    9.026455] iwlwifi 0000:02:00.0: Direct firmware load for iwlwifi-8000C-32.ucode failed with error -2
[    9.348325] iwlwifi 0000:02:00.0: loaded firmware version 31.532993.0 op_mode iwlmvm
[    9.369307] iwlwifi 0000:02:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208
[    9.435915] iwlwifi 0000:02:00.0: base HW address: 34:13:e8:2d:65:ef
[    9.509217] ieee80211 phy0: Selected rate control algorithm 'iwl-mvm-rs'


So this seems to be something that doesn't happen in all machines, maybe
a PCIe problem?

Is there any chance you could try to bisect this?

--
Cheers,
Luca.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-12 16:36   ` Luca Coelho
@ 2017-09-12 16:59     ` Jens Axboe
  2017-09-12 19:43     ` Jens Axboe
  1 sibling, 0 replies; 31+ messages in thread
From: Jens Axboe @ 2017-09-12 16:59 UTC (permalink / raw)
  To: Luca Coelho, Grumbach, Emmanuel, Berg, Johannes; +Cc: linuxwifi, linux-wireless

On 09/12/2017 10:36 AM, Luca Coelho wrote:
> On Tue, 2017-09-12 at 16:11 +0000, Coelho, Luciano wrote:
>> Hi Jens,
>>
>> On Tue, 2017-09-12 at 09:48 -0600, Jens Axboe wrote:
>>> Hi,
>>>
>>> I have no wifi in current git (8fac2f96ab8), it simply fails with:
>>>
>>> [    4.363481] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-34.ucode failed with error -2
>>> [    4.363733] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-33.ucode failed with error -2
>>> [    4.363744] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-32.ucode failed with error -2
>>> [    4.368077] iwlwifi 0000:04:00.0: loaded firmware version 31.532993.0 op_mode iwlmvm
>>> [    4.461753] iwlwifi 0000:04:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208
>>> [ ... ]
>>> [    9.510570] iwlwifi 0000:04:00.0: Failed to load firmware chunk!             
>>> [    9.513903] iwlwifi 0000:04:00.0: Could not load the [0] uCode section       
>>> [    9.516201] iwlwifi 0000:04:00.0: Failed to start INIT ucode: -110           
>>> [    9.530842] iwlwifi 0000:04:00.0: Failed to run INIT ucode: -110             
>>>
>>> I get the same thing with -27 of the firmware:
>>>
>>> [   60.990831] Intel(R) Wireless WiFi driver for Linux                          
>>> [   60.990833] Copyright(c) 2003- 2015 Intel Corporation                        
>>> [   60.991936] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-34.ucode failed with error -2
>>> [   60.991941] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-33.ucode failed with error -2
>>> [   60.991946] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-32.ucode failed with error -2
>>> [   60.991957] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-31.ucode failed with error -2
>>> [   60.991964] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-30.ucode failed with error -2
>>> [   60.991969] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-29.ucode failed with error -2
>>> [   60.991975] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-28.ucode failed with error -2
>>> [   61.029852] iwlwifi 0000:04:00.0: loaded firmware version 27.541033.0 op_mode iwlmvm
>>> [   61.043660] iwlwifi 0000:04:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208
>>> [   66.070135] iwlwifi 0000:04:00.0: Failed to load firmware chunk!             
>>> [   66.070149] iwlwifi 0000:04:00.0: Could not load the [0] uCode section       
>>> [   66.070165] iwlwifi 0000:04:00.0: Failed to start INIT ucode: -110           
>>> [   66.083462] iwlwifi 0000:04:00.0: Failed to run INIT ucode: -110             
>>>
>>> Things work fine in 4.13-rc7+, which was the last kernel I ran on my laptop.
>>
>> This is strange, Linus has been running this same combination with -31
>> (with -27 we had a regression, but it was fixed recently and the fix is
>> in the current master).  I have also tried this combination with both
>> -27 and -31 after my fix and it works fine.
>>
>> There are only a couple of other patches that may affect iwlwifi since
>> the previous net-next.git pull request...
>>
>> I'll try this combination on my machine and see if I can reproduce the
>> problem.
> 
> I just booted my machine with the latest linux.git/master (8fac2f96ab86)
> and it works without a problem:
> 
> [    8.902174] Intel(R) Wireless WiFi driver for Linux
> [    8.902174] Copyright(c) 2003- 2015 Intel Corporation
> [    8.995112] iwlwifi 0000:02:00.0: Direct firmware load for iwlwifi-8000C-34.ucode failed with error -2
> [    8.995136] iwlwifi 0000:02:00.0: Direct firmware load for iwlwifi-8000C-33.ucode failed with error -2
> [    9.026455] iwlwifi 0000:02:00.0: Direct firmware load for iwlwifi-8000C-32.ucode failed with error -2
> [    9.348325] iwlwifi 0000:02:00.0: loaded firmware version 31.532993.0 op_mode iwlmvm
> [    9.369307] iwlwifi 0000:02:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208
> [    9.435915] iwlwifi 0000:02:00.0: base HW address: 34:13:e8:2d:65:ef
> [    9.509217] ieee80211 phy0: Selected rate control algorithm 'iwl-mvm-rs'
> 
> 
> So this seems to be something that doesn't happen in all machines, maybe
> a PCIe problem?
> 
> Is there any chance you could try to bisect this?

I can try, but it might be an all day thing on the laptop. I'll get it going.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-12 16:36   ` Luca Coelho
  2017-09-12 16:59     ` Jens Axboe
@ 2017-09-12 19:43     ` Jens Axboe
  2017-09-12 19:51       ` Luca Coelho
  2017-09-12 20:04       ` Johannes Berg
  1 sibling, 2 replies; 31+ messages in thread
From: Jens Axboe @ 2017-09-12 19:43 UTC (permalink / raw)
  To: Luca Coelho, Grumbach, Emmanuel, Berg, Johannes
  Cc: linuxwifi, linux-wireless, srinath.mannam, Bjorn Helgaas

On 09/12/2017 10:36 AM, Luca Coelho wrote:
> On Tue, 2017-09-12 at 16:11 +0000, Coelho, Luciano wrote:
>> Hi Jens,
>>
>> On Tue, 2017-09-12 at 09:48 -0600, Jens Axboe wrote:
>>> Hi,
>>>
>>> I have no wifi in current git (8fac2f96ab8), it simply fails with:
>>>
>>> [    4.363481] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-34.ucode failed with error -2
>>> [    4.363733] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-33.ucode failed with error -2
>>> [    4.363744] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-32.ucode failed with error -2
>>> [    4.368077] iwlwifi 0000:04:00.0: loaded firmware version 31.532993.0 op_mode iwlmvm
>>> [    4.461753] iwlwifi 0000:04:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208
>>> [ ... ]
>>> [    9.510570] iwlwifi 0000:04:00.0: Failed to load firmware chunk!             
>>> [    9.513903] iwlwifi 0000:04:00.0: Could not load the [0] uCode section       
>>> [    9.516201] iwlwifi 0000:04:00.0: Failed to start INIT ucode: -110           
>>> [    9.530842] iwlwifi 0000:04:00.0: Failed to run INIT ucode: -110             
>>>
>>> I get the same thing with -27 of the firmware:
>>>
>>> [   60.990831] Intel(R) Wireless WiFi driver for Linux                          
>>> [   60.990833] Copyright(c) 2003- 2015 Intel Corporation                        
>>> [   60.991936] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-34.ucode failed with error -2
>>> [   60.991941] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-33.ucode failed with error -2
>>> [   60.991946] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-32.ucode failed with error -2
>>> [   60.991957] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-31.ucode failed with error -2
>>> [   60.991964] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-30.ucode failed with error -2
>>> [   60.991969] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-29.ucode failed with error -2
>>> [   60.991975] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-28.ucode failed with error -2
>>> [   61.029852] iwlwifi 0000:04:00.0: loaded firmware version 27.541033.0 op_mode iwlmvm
>>> [   61.043660] iwlwifi 0000:04:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208
>>> [   66.070135] iwlwifi 0000:04:00.0: Failed to load firmware chunk!             
>>> [   66.070149] iwlwifi 0000:04:00.0: Could not load the [0] uCode section       
>>> [   66.070165] iwlwifi 0000:04:00.0: Failed to start INIT ucode: -110           
>>> [   66.083462] iwlwifi 0000:04:00.0: Failed to run INIT ucode: -110             
>>>
>>> Things work fine in 4.13-rc7+, which was the last kernel I ran on my laptop.
>>
>> This is strange, Linus has been running this same combination with -31
>> (with -27 we had a regression, but it was fixed recently and the fix is
>> in the current master).  I have also tried this combination with both
>> -27 and -31 after my fix and it works fine.
>>
>> There are only a couple of other patches that may affect iwlwifi since
>> the previous net-next.git pull request...
>>
>> I'll try this combination on my machine and see if I can reproduce the
>> problem.
> 
> I just booted my machine with the latest linux.git/master (8fac2f96ab86)
> and it works without a problem:
> 
> [    8.902174] Intel(R) Wireless WiFi driver for Linux
> [    8.902174] Copyright(c) 2003- 2015 Intel Corporation
> [    8.995112] iwlwifi 0000:02:00.0: Direct firmware load for iwlwifi-8000C-34.ucode failed with error -2
> [    8.995136] iwlwifi 0000:02:00.0: Direct firmware load for iwlwifi-8000C-33.ucode failed with error -2
> [    9.026455] iwlwifi 0000:02:00.0: Direct firmware load for iwlwifi-8000C-32.ucode failed with error -2
> [    9.348325] iwlwifi 0000:02:00.0: loaded firmware version 31.532993.0 op_mode iwlmvm
> [    9.369307] iwlwifi 0000:02:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208
> [    9.435915] iwlwifi 0000:02:00.0: base HW address: 34:13:e8:2d:65:ef
> [    9.509217] ieee80211 phy0: Selected rate control algorithm 'iwl-mvm-rs'
> 
> 
> So this seems to be something that doesn't happen in all machines, maybe
> a PCIe problem?
> 
> Is there any chance you could try to bisect this?

Bisect done, it tells me that this:

commit 40f11adc7cd9281227f0a6a627d966dd0a5f0cd9
Author: Srinath Mannam <srinath.mannam@broadcom.com>
Date:   Fri Aug 18 21:50:48 2017 -0500

    PCI: Avoid race while enabling upstream bridges

is the first bad commit. Reverting that on top of master makes things
work fine again, so that commit is definitely b0rken.

CC'ing the guilty part and Bjorn. I'm assuming it's the pci_is_enabled()
check, since the rest of the patch shouldn't have functional changes.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-12 19:43     ` Jens Axboe
@ 2017-09-12 19:51       ` Luca Coelho
  2017-09-12 20:04       ` Johannes Berg
  1 sibling, 0 replies; 31+ messages in thread
From: Luca Coelho @ 2017-09-12 19:51 UTC (permalink / raw)
  To: Jens Axboe, Grumbach, Emmanuel, Berg, Johannes
  Cc: linuxwifi, linux-wireless, srinath.mannam, Bjorn Helgaas

On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
> On 09/12/2017 10:36 AM, Luca Coelho wrote:
> > On Tue, 2017-09-12 at 16:11 +0000, Coelho, Luciano wrote:
> > > Hi Jens,
> > > 
> > > On Tue, 2017-09-12 at 09:48 -0600, Jens Axboe wrote:
> > > > Hi,
> > > > 
> > > > I have no wifi in current git (8fac2f96ab8), it simply fails with:
> > > > 
> > > > [    4.363481] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-34.ucode failed with error -2
> > > > [    4.363733] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-33.ucode failed with error -2
> > > > [    4.363744] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-32.ucode failed with error -2
> > > > [    4.368077] iwlwifi 0000:04:00.0: loaded firmware version 31.532993.0 op_mode iwlmvm
> > > > [    4.461753] iwlwifi 0000:04:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208
> > > > [ ... ]
> > > > [    9.510570] iwlwifi 0000:04:00.0: Failed to load firmware chunk!             
> > > > [    9.513903] iwlwifi 0000:04:00.0: Could not load the [0] uCode section       
> > > > [    9.516201] iwlwifi 0000:04:00.0: Failed to start INIT ucode: -110           
> > > > [    9.530842] iwlwifi 0000:04:00.0: Failed to run INIT ucode: -110             
> > > > 
> > > > I get the same thing with -27 of the firmware:
> > > > 
> > > > [   60.990831] Intel(R) Wireless WiFi driver for Linux                          
> > > > [   60.990833] Copyright(c) 2003- 2015 Intel Corporation                        
> > > > [   60.991936] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-34.ucode failed with error -2
> > > > [   60.991941] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-33.ucode failed with error -2
> > > > [   60.991946] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-32.ucode failed with error -2
> > > > [   60.991957] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-31.ucode failed with error -2
> > > > [   60.991964] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-30.ucode failed with error -2
> > > > [   60.991969] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-29.ucode failed with error -2
> > > > [   60.991975] iwlwifi 0000:04:00.0: Direct firmware load for iwlwifi-8000C-28.ucode failed with error -2
> > > > [   61.029852] iwlwifi 0000:04:00.0: loaded firmware version 27.541033.0 op_mode iwlmvm
> > > > [   61.043660] iwlwifi 0000:04:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208
> > > > [   66.070135] iwlwifi 0000:04:00.0: Failed to load firmware chunk!             
> > > > [   66.070149] iwlwifi 0000:04:00.0: Could not load the [0] uCode section       
> > > > [   66.070165] iwlwifi 0000:04:00.0: Failed to start INIT ucode: -110           
> > > > [   66.083462] iwlwifi 0000:04:00.0: Failed to run INIT ucode: -110             
> > > > 
> > > > Things work fine in 4.13-rc7+, which was the last kernel I ran on my laptop.
> > > 
> > > This is strange, Linus has been running this same combination with -31
> > > (with -27 we had a regression, but it was fixed recently and the fix is
> > > in the current master).  I have also tried this combination with both
> > > -27 and -31 after my fix and it works fine.
> > > 
> > > There are only a couple of other patches that may affect iwlwifi since
> > > the previous net-next.git pull request...
> > > 
> > > I'll try this combination on my machine and see if I can reproduce the
> > > problem.
> > 
> > I just booted my machine with the latest linux.git/master (8fac2f96ab86)
> > and it works without a problem:
> > 
> > [    8.902174] Intel(R) Wireless WiFi driver for Linux
> > [    8.902174] Copyright(c) 2003- 2015 Intel Corporation
> > [    8.995112] iwlwifi 0000:02:00.0: Direct firmware load for iwlwifi-8000C-34.ucode failed with error -2
> > [    8.995136] iwlwifi 0000:02:00.0: Direct firmware load for iwlwifi-8000C-33.ucode failed with error -2
> > [    9.026455] iwlwifi 0000:02:00.0: Direct firmware load for iwlwifi-8000C-32.ucode failed with error -2
> > [    9.348325] iwlwifi 0000:02:00.0: loaded firmware version 31.532993.0 op_mode iwlmvm
> > [    9.369307] iwlwifi 0000:02:00.0: Detected Intel(R) Dual Band Wireless AC 8260, REV=0x208
> > [    9.435915] iwlwifi 0000:02:00.0: base HW address: 34:13:e8:2d:65:ef
> > [    9.509217] ieee80211 phy0: Selected rate control algorithm 'iwl-mvm-rs'
> > 
> > 
> > So this seems to be something that doesn't happen in all machines, maybe
> > a PCIe problem?
> > 
> > Is there any chance you could try to bisect this?
> 
> Bisect done, it tells me that this:
> 
> commit 40f11adc7cd9281227f0a6a627d966dd0a5f0cd9
> Author: Srinath Mannam <srinath.mannam@broadcom.com>
> Date:   Fri Aug 18 21:50:48 2017 -0500
> 
>     PCI: Avoid race while enabling upstream bridges
> 
> is the first bad commit. Reverting that on top of master makes things
> work fine again, so that commit is definitely b0rken.
> 
> CC'ing the guilty part and Bjorn. I'm assuming it's the pci_is_enabled()
> check, since the rest of the patch shouldn't have functional changes.

Great! I'm glad that you could track it down to a single commit.

Bjorn/Srinath, let me know if it is something that the iwlwifi is not
doing properly.  But AFAICT the !pci_is_enabled() is checking the bridge
 *above* the iwlwifi device, so I don't think that will be the case.

--
Cheers,
Luca.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-12 19:43     ` Jens Axboe
  2017-09-12 19:51       ` Luca Coelho
@ 2017-09-12 20:04       ` Johannes Berg
  2017-09-14 17:00         ` Jens Axboe
  1 sibling, 1 reply; 31+ messages in thread
From: Johannes Berg @ 2017-09-12 20:04 UTC (permalink / raw)
  To: Jens Axboe, Luca Coelho, Grumbach, Emmanuel
  Cc: linuxwifi, linux-wireless, srinath.mannam, Bjorn Helgaas

On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:

> CC'ing the guilty part and Bjorn. I'm assuming it's the
> pci_is_enabled() check, since the rest of the patch shouldn't have
> functional changes.

and pci_enable_bridge() already checks if it's already enabled, but
still enables mastering in that case if it isn't:

static void pci_enable_bridge(struct pci_dev *dev)
{
[...]
        if (pci_is_enabled(dev)) {
                if (!dev->is_busmaster)
                        pci_set_master(dev);
                return;
        }

so I guess due to the new check we end up with mastering disabled, and
thus the firmware can't load since that's a DMA thing?

johannes

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-12 20:04       ` Johannes Berg
@ 2017-09-14 17:00         ` Jens Axboe
  2017-09-14 17:11           ` Bjorn Helgaas
  0 siblings, 1 reply; 31+ messages in thread
From: Jens Axboe @ 2017-09-14 17:00 UTC (permalink / raw)
  To: Johannes Berg, Luca Coelho, Grumbach, Emmanuel
  Cc: linuxwifi, linux-wireless, srinath.mannam, Bjorn Helgaas

On 09/12/2017 02:04 PM, Johannes Berg wrote:
> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
> 
>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>> pci_is_enabled() check, since the rest of the patch shouldn't have
>> functional changes.
> 
> and pci_enable_bridge() already checks if it's already enabled, but
> still enables mastering in that case if it isn't:
> 
> static void pci_enable_bridge(struct pci_dev *dev)
> {
> [...]
>         if (pci_is_enabled(dev)) {
>                 if (!dev->is_busmaster)
>                         pci_set_master(dev);
>                 return;
>         }
> 
> so I guess due to the new check we end up with mastering disabled, and
> thus the firmware can't load since that's a DMA thing?

Bjorn/Srinath, any input here? This is a regression that prevents wifi
from working on a pretty standard laptop. It'd suck to have this be in
-rc1. Seems like the trivial fix would be:


diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b0002daa50f3..ffbe11dbdd61 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
 		return 0;		/* already enabled */
 
 	bridge = pci_upstream_bridge(dev);
-	if (bridge && !pci_is_enabled(bridge))
+	if (bridge)
 		pci_enable_bridge(bridge);
 
 	/* only skip sriov related */


-- 
Jens Axboe

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-14 17:00         ` Jens Axboe
@ 2017-09-14 17:11           ` Bjorn Helgaas
  2017-09-14 17:22             ` Jens Axboe
  0 siblings, 1 reply; 31+ messages in thread
From: Bjorn Helgaas @ 2017-09-14 17:11 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Johannes Berg, Luca Coelho, Grumbach, Emmanuel, linuxwifi,
	linux-wireless, srinath.mannam, linux-pci

[+cc linux-pci]

On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 09/12/2017 02:04 PM, Johannes Berg wrote:
>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>>
>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>>> pci_is_enabled() check, since the rest of the patch shouldn't have
>>> functional changes.
>>
>> and pci_enable_bridge() already checks if it's already enabled, but
>> still enables mastering in that case if it isn't:
>>
>> static void pci_enable_bridge(struct pci_dev *dev)
>> {
>> [...]
>>         if (pci_is_enabled(dev)) {
>>                 if (!dev->is_busmaster)
>>                         pci_set_master(dev);
>>                 return;
>>         }
>>
>> so I guess due to the new check we end up with mastering disabled, and
>> thus the firmware can't load since that's a DMA thing?
>
> Bjorn/Srinath, any input here? This is a regression that prevents wifi
> from working on a pretty standard laptop. It'd suck to have this be in
> -rc1. Seems like the trivial fix would be:
>
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b0002daa50f3..ffbe11dbdd61 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>                 return 0;               /* already enabled */
>
>         bridge = pci_upstream_bridge(dev);
> -       if (bridge && !pci_is_enabled(bridge))
> +       if (bridge)
>                 pci_enable_bridge(bridge);
>
>         /* only skip sriov related */
>
>

Looks like a reasonable fix.  I assume it works for you?  I don't have
a way to test it, so if you can verify that it works and supply a
Signed-off-by, I can merge it.

Bjorn

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-14 17:11           ` Bjorn Helgaas
@ 2017-09-14 17:22             ` Jens Axboe
  2017-09-14 17:28               ` Srinath Mannam
  0 siblings, 1 reply; 31+ messages in thread
From: Jens Axboe @ 2017-09-14 17:22 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Johannes Berg, Luca Coelho, Grumbach, Emmanuel, linuxwifi,
	linux-wireless, srinath.mannam, linux-pci

On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
> [+cc linux-pci]
> 
> On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 09/12/2017 02:04 PM, Johannes Berg wrote:
>>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>>>
>>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>>>> pci_is_enabled() check, since the rest of the patch shouldn't have
>>>> functional changes.
>>>
>>> and pci_enable_bridge() already checks if it's already enabled, but
>>> still enables mastering in that case if it isn't:
>>>
>>> static void pci_enable_bridge(struct pci_dev *dev)
>>> {
>>> [...]
>>>         if (pci_is_enabled(dev)) {
>>>                 if (!dev->is_busmaster)
>>>                         pci_set_master(dev);
>>>                 return;
>>>         }
>>>
>>> so I guess due to the new check we end up with mastering disabled, and
>>> thus the firmware can't load since that's a DMA thing?
>>
>> Bjorn/Srinath, any input here? This is a regression that prevents wifi
>> from working on a pretty standard laptop. It'd suck to have this be in
>> -rc1. Seems like the trivial fix would be:
>>
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index b0002daa50f3..ffbe11dbdd61 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>>                 return 0;               /* already enabled */
>>
>>         bridge = pci_upstream_bridge(dev);
>> -       if (bridge && !pci_is_enabled(bridge))
>> +       if (bridge)
>>                 pci_enable_bridge(bridge);
>>
>>         /* only skip sriov related */
>>
>>
> 
> Looks like a reasonable fix.  I assume it works for you?  I don't have
> a way to test it, so if you can verify that it works and supply a
> Signed-off-by, I can merge it.

Booting it right now, I'll send out a proper version in a few minutes.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-14 17:22             ` Jens Axboe
@ 2017-09-14 17:28               ` Srinath Mannam
  2017-09-14 17:35                 ` Jens Axboe
  0 siblings, 1 reply; 31+ messages in thread
From: Srinath Mannam @ 2017-09-14 17:28 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Bjorn Helgaas, Johannes Berg, Luca Coelho, Grumbach, Emmanuel,
	linuxwifi, linux-wireless, linux-pci

Hi Bjorn,

On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe <axboe@kernel.dk> wrote:
>
> On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
> > [+cc linux-pci]
> >
> > On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
> >> On 09/12/2017 02:04 PM, Johannes Berg wrote:
> >>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
> >>>
> >>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
> >>>> pci_is_enabled() check, since the rest of the patch shouldn't have
> >>>> functional changes.
> >>>
> >>> and pci_enable_bridge() already checks if it's already enabled, but
> >>> still enables mastering in that case if it isn't:
> >>>
> >>> static void pci_enable_bridge(struct pci_dev *dev)
> >>> {
> >>> [...]
> >>>         if (pci_is_enabled(dev)) {
> >>>                 if (!dev->is_busmaster)
> >>>                         pci_set_master(dev);
> >>>                 return;
> >>>         }
> >>>
> >>> so I guess due to the new check we end up with mastering disabled, and
> >>> thus the firmware can't load since that's a DMA thing?
> >>
> >> Bjorn/Srinath, any input here? This is a regression that prevents wifi
> >> from working on a pretty standard laptop. It'd suck to have this be in
> >> -rc1. Seems like the trivial fix would be:
> >>
> >>
> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >> index b0002daa50f3..ffbe11dbdd61 100644
> >> --- a/drivers/pci/pci.c
> >> +++ b/drivers/pci/pci.c
> >> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
> >>                 return 0;               /* already enabled */
> >>
> >>         bridge = pci_upstream_bridge(dev);
> >> -       if (bridge && !pci_is_enabled(bridge))
> >> +       if (bridge)
With this change and keeping "mutex_lock(&pci_bridge_mutex);" in
pci_enable_bridge functoin will causes a nexted lock.

> >>                 pci_enable_bridge(bridge);
> >>
> >>         /* only skip sriov related */
> >>
> >>
> >
> > Looks like a reasonable fix.  I assume it works for you?  I don't have
> > a way to test it, so if you can verify that it works and supply a
> > Signed-off-by, I can merge it.
>
> Booting it right now, I'll send out a proper version in a few minutes.
>
> --
> Jens Axboe
>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-14 17:28               ` Srinath Mannam
@ 2017-09-14 17:35                 ` Jens Axboe
  2017-09-14 17:44                   ` Srinath Mannam
  2017-09-14 17:44                   ` Jens Axboe
  0 siblings, 2 replies; 31+ messages in thread
From: Jens Axboe @ 2017-09-14 17:35 UTC (permalink / raw)
  To: Srinath Mannam
  Cc: Bjorn Helgaas, Johannes Berg, Luca Coelho, Grumbach, Emmanuel,
	linuxwifi, linux-wireless, linux-pci

On 09/14/2017 11:28 AM, Srinath Mannam wrote:
> Hi Bjorn,
> 
> On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
>>> [+cc linux-pci]
>>>
>>> On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 09/12/2017 02:04 PM, Johannes Berg wrote:
>>>>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>>>>>
>>>>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>>>>>> pci_is_enabled() check, since the rest of the patch shouldn't have
>>>>>> functional changes.
>>>>>
>>>>> and pci_enable_bridge() already checks if it's already enabled, but
>>>>> still enables mastering in that case if it isn't:
>>>>>
>>>>> static void pci_enable_bridge(struct pci_dev *dev)
>>>>> {
>>>>> [...]
>>>>>         if (pci_is_enabled(dev)) {
>>>>>                 if (!dev->is_busmaster)
>>>>>                         pci_set_master(dev);
>>>>>                 return;
>>>>>         }
>>>>>
>>>>> so I guess due to the new check we end up with mastering disabled, and
>>>>> thus the firmware can't load since that's a DMA thing?
>>>>
>>>> Bjorn/Srinath, any input here? This is a regression that prevents wifi
>>>> from working on a pretty standard laptop. It'd suck to have this be in
>>>> -rc1. Seems like the trivial fix would be:
>>>>
>>>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index b0002daa50f3..ffbe11dbdd61 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>>>>                 return 0;               /* already enabled */
>>>>
>>>>         bridge = pci_upstream_bridge(dev);
>>>> -       if (bridge && !pci_is_enabled(bridge))
>>>> +       if (bridge)
> With this change and keeping "mutex_lock(&pci_bridge_mutex);" in
> pci_enable_bridge functoin will causes a nexted lock.

Took a look, and looks like you are right. That code looks like a mess,
fwiw.

I'd strongly suggest that the bad commit is reverted until a proper
solution is found, since the simple one-liner could potentially
introduce a deadlock with your patch applied.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-14 17:35                 ` Jens Axboe
@ 2017-09-14 17:44                   ` Srinath Mannam
  2017-09-14 17:45                     ` Jens Axboe
  2017-09-14 17:50                     ` Johannes Berg
  2017-09-14 17:44                   ` Jens Axboe
  1 sibling, 2 replies; 31+ messages in thread
From: Srinath Mannam @ 2017-09-14 17:44 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Bjorn Helgaas, Johannes Berg, Luca Coelho, Grumbach, Emmanuel,
	linuxwifi, linux-wireless, linux-pci

Hi Jens Axboe,

On Thu, Sep 14, 2017 at 11:05 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 09/14/2017 11:28 AM, Srinath Mannam wrote:
>> Hi Bjorn,
>>
>> On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
>>>> [+cc linux-pci]
>>>>
>>>> On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>> On 09/12/2017 02:04 PM, Johannes Berg wrote:
>>>>>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>>>>>>
>>>>>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>>>>>>> pci_is_enabled() check, since the rest of the patch shouldn't have
>>>>>>> functional changes.
>>>>>>
>>>>>> and pci_enable_bridge() already checks if it's already enabled, but
>>>>>> still enables mastering in that case if it isn't:
>>>>>>
>>>>>> static void pci_enable_bridge(struct pci_dev *dev)
>>>>>> {
>>>>>> [...]
>>>>>>         if (pci_is_enabled(dev)) {
>>>>>>                 if (!dev->is_busmaster)
>>>>>>                         pci_set_master(dev);
>>>>>>                 return;
>>>>>>         }
>>>>>>
>>>>>> so I guess due to the new check we end up with mastering disabled, and
>>>>>> thus the firmware can't load since that's a DMA thing?
>>>>>
>>>>> Bjorn/Srinath, any input here? This is a regression that prevents wifi
>>>>> from working on a pretty standard laptop. It'd suck to have this be in
>>>>> -rc1. Seems like the trivial fix would be:
>>>>>
>>>>>
>>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>>> index b0002daa50f3..ffbe11dbdd61 100644
>>>>> --- a/drivers/pci/pci.c
>>>>> +++ b/drivers/pci/pci.c
>>>>> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>>>>>                 return 0;               /* already enabled */
>>>>>
>>>>>         bridge = pci_upstream_bridge(dev);
>>>>> -       if (bridge && !pci_is_enabled(bridge))
>>>>> +       if (bridge)
>> With this change and keeping "mutex_lock(&pci_bridge_mutex);" in
>> pci_enable_bridge functoin will causes a nexted lock.
>
> Took a look, and looks like you are right. That code looks like a mess,
> fwiw.
>
> I'd strongly suggest that the bad commit is reverted until a proper
> solution is found, since the simple one-liner could potentially
> introduce a deadlock with your patch applied.
atomic_inc_return(&dev->enable_cnt) in function
pci_enable_device_flags enables passed pcie device.
!pci_is_enabled(bridge) check in "if (bridge &&
!pci_is_enabled(bridge))"  checks for bridge device of previous pcie
device.
So it will not stop bus_master of bridge device.
In your case how many bridges in between endpoint and host bridge?
Please provide the details about your use case.
Thank you.
>
> --
> Jens Axboe
>
Regards,
Srinath.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-14 17:35                 ` Jens Axboe
  2017-09-14 17:44                   ` Srinath Mannam
@ 2017-09-14 17:44                   ` Jens Axboe
  2017-09-14 20:04                     ` Srinath Mannam
  1 sibling, 1 reply; 31+ messages in thread
From: Jens Axboe @ 2017-09-14 17:44 UTC (permalink / raw)
  To: Srinath Mannam
  Cc: Bjorn Helgaas, Johannes Berg, Luca Coelho, Grumbach, Emmanuel,
	linuxwifi, linux-wireless, linux-pci

On 09/14/2017 11:35 AM, Jens Axboe wrote:
> On 09/14/2017 11:28 AM, Srinath Mannam wrote:
>> Hi Bjorn,
>>
>> On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
>>>> [+cc linux-pci]
>>>>
>>>> On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>> On 09/12/2017 02:04 PM, Johannes Berg wrote:
>>>>>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>>>>>>
>>>>>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>>>>>>> pci_is_enabled() check, since the rest of the patch shouldn't have
>>>>>>> functional changes.
>>>>>>
>>>>>> and pci_enable_bridge() already checks if it's already enabled, but
>>>>>> still enables mastering in that case if it isn't:
>>>>>>
>>>>>> static void pci_enable_bridge(struct pci_dev *dev)
>>>>>> {
>>>>>> [...]
>>>>>>         if (pci_is_enabled(dev)) {
>>>>>>                 if (!dev->is_busmaster)
>>>>>>                         pci_set_master(dev);
>>>>>>                 return;
>>>>>>         }
>>>>>>
>>>>>> so I guess due to the new check we end up with mastering disabled, and
>>>>>> thus the firmware can't load since that's a DMA thing?
>>>>>
>>>>> Bjorn/Srinath, any input here? This is a regression that prevents wifi
>>>>> from working on a pretty standard laptop. It'd suck to have this be in
>>>>> -rc1. Seems like the trivial fix would be:
>>>>>
>>>>>
>>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>>> index b0002daa50f3..ffbe11dbdd61 100644
>>>>> --- a/drivers/pci/pci.c
>>>>> +++ b/drivers/pci/pci.c
>>>>> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>>>>>                 return 0;               /* already enabled */
>>>>>
>>>>>         bridge = pci_upstream_bridge(dev);
>>>>> -       if (bridge && !pci_is_enabled(bridge))
>>>>> +       if (bridge)
>> With this change and keeping "mutex_lock(&pci_bridge_mutex);" in
>> pci_enable_bridge functoin will causes a nexted lock.
> 
> Took a look, and looks like you are right. That code looks like a mess,
> fwiw.
> 
> I'd strongly suggest that the bad commit is reverted until a proper
> solution is found, since the simple one-liner could potentially
> introduce a deadlock with your patch applied.

BTW, your patch looks pretty bad too, introducing a random mutex
deep on code that can be recursive. Why isn't this check in
pci_enable_device_flags() enough to prevent double-enable of
an upstream bridge?

if (atomic_inc_return(&dev->enable_cnt) > 1)                            
	return 0;               /* already enabled */

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-14 17:44                   ` Srinath Mannam
@ 2017-09-14 17:45                     ` Jens Axboe
  2017-09-14 17:50                     ` Johannes Berg
  1 sibling, 0 replies; 31+ messages in thread
From: Jens Axboe @ 2017-09-14 17:45 UTC (permalink / raw)
  To: Srinath Mannam
  Cc: Bjorn Helgaas, Johannes Berg, Luca Coelho, Grumbach, Emmanuel,
	linuxwifi, linux-wireless, linux-pci

On 09/14/2017 11:44 AM, Srinath Mannam wrote:
> Hi Jens Axboe,
> 
> On Thu, Sep 14, 2017 at 11:05 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 09/14/2017 11:28 AM, Srinath Mannam wrote:
>>> Hi Bjorn,
>>>
>>> On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>
>>>> On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
>>>>> [+cc linux-pci]
>>>>>
>>>>> On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>>> On 09/12/2017 02:04 PM, Johannes Berg wrote:
>>>>>>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>>>>>>>
>>>>>>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>>>>>>>> pci_is_enabled() check, since the rest of the patch shouldn't have
>>>>>>>> functional changes.
>>>>>>>
>>>>>>> and pci_enable_bridge() already checks if it's already enabled, but
>>>>>>> still enables mastering in that case if it isn't:
>>>>>>>
>>>>>>> static void pci_enable_bridge(struct pci_dev *dev)
>>>>>>> {
>>>>>>> [...]
>>>>>>>         if (pci_is_enabled(dev)) {
>>>>>>>                 if (!dev->is_busmaster)
>>>>>>>                         pci_set_master(dev);
>>>>>>>                 return;
>>>>>>>         }
>>>>>>>
>>>>>>> so I guess due to the new check we end up with mastering disabled, and
>>>>>>> thus the firmware can't load since that's a DMA thing?
>>>>>>
>>>>>> Bjorn/Srinath, any input here? This is a regression that prevents wifi
>>>>>> from working on a pretty standard laptop. It'd suck to have this be in
>>>>>> -rc1. Seems like the trivial fix would be:
>>>>>>
>>>>>>
>>>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>>>> index b0002daa50f3..ffbe11dbdd61 100644
>>>>>> --- a/drivers/pci/pci.c
>>>>>> +++ b/drivers/pci/pci.c
>>>>>> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>>>>>>                 return 0;               /* already enabled */
>>>>>>
>>>>>>         bridge = pci_upstream_bridge(dev);
>>>>>> -       if (bridge && !pci_is_enabled(bridge))
>>>>>> +       if (bridge)
>>> With this change and keeping "mutex_lock(&pci_bridge_mutex);" in
>>> pci_enable_bridge functoin will causes a nexted lock.
>>
>> Took a look, and looks like you are right. That code looks like a mess,
>> fwiw.
>>
>> I'd strongly suggest that the bad commit is reverted until a proper
>> solution is found, since the simple one-liner could potentially
>> introduce a deadlock with your patch applied.
> atomic_inc_return(&dev->enable_cnt) in function
> pci_enable_device_flags enables passed pcie device.
> !pci_is_enabled(bridge) check in "if (bridge &&
> !pci_is_enabled(bridge))"  checks for bridge device of previous pcie
> device.
> So it will not stop bus_master of bridge device.
> In your case how many bridges in between endpoint and host bridge?
> Please provide the details about your use case.

It's a bog standard Lenovo X1 Carbon, gen4.

# lspci
00:00.0 Host bridge: Intel Corporation Sky Lake Host Bridge/DRAM Registers (rev 08)
00:02.0 VGA compatible controller: Intel Corporation Sky Lake Integrated Graphics (rev 07)
00:08.0 System peripheral: Intel Corporation Sky Lake Gaussian Mixture Model
00:13.0 Non-VGA unclassified device: Intel Corporation Device 9d35 (rev 21)
00:14.0 USB controller: Intel Corporation Sunrise Point-LP USB 3.0 xHCI Controller (rev 21)
00:14.2 Signal processing controller: Intel Corporation Sunrise Point-LP Thermal subsystem (rev 21)
00:16.0 Communication controller: Intel Corporation Sunrise Point-LP CSME HECI (rev 21)
00:1c.0 PCI bridge: Intel Corporation Device 9d10 (rev f1)
00:1c.2 PCI bridge: Intel Corporation Device 9d12 (rev f1)
00:1c.4 PCI bridge: Intel Corporation Sunrise Point-LP PCI Express Root Port (rev f1)
00:1f.0 ISA bridge: Intel Corporation Sunrise Point-LP LPC Controller (rev 21)
00:1f.2 Memory controller: Intel Corporation Sunrise Point-LP PMC (rev 21)
00:1f.3 Audio device: Intel Corporation Sunrise Point-LP HD Audio (rev 21)
00:1f.4 SMBus: Intel Corporation Sunrise Point-LP SMBus (rev 21)
00:1f.6 Ethernet controller: Intel Corporation Ethernet Connection I219-LM (rev 21)
04:00.0 Network controller: Intel Corporation Wireless 8260 (rev 3a)
05:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd NVMe SSD Controller (rev 01)

# lspci -t
-[0000:00]-+-00.0
           +-02.0
           +-08.0
           +-13.0
           +-14.0
           +-14.2
           +-16.0
           +-1c.0-[02]--
           +-1c.2-[04]----00.0
           +-1c.4-[05]----00.0
           +-1f.0
           +-1f.2
           +-1f.3
           +-1f.4
           \-1f.6

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-14 17:44                   ` Srinath Mannam
  2017-09-14 17:45                     ` Jens Axboe
@ 2017-09-14 17:50                     ` Johannes Berg
  1 sibling, 0 replies; 31+ messages in thread
From: Johannes Berg @ 2017-09-14 17:50 UTC (permalink / raw)
  To: Srinath Mannam, Jens Axboe
  Cc: Bjorn Helgaas, Luca Coelho, Grumbach, Emmanuel, linuxwifi,
	linux-wireless, linux-pci

On Thu, 2017-09-14 at 23:14 +0530, Srinath Mannam wrote:
> 
> atomic_inc_return(&dev->enable_cnt) in function
> pci_enable_device_flags enables passed pcie device.
> !pci_is_enabled(bridge) check in "if (bridge &&
> !pci_is_enabled(bridge))"  checks for bridge device of previous pcie
> device.
> So it will not stop bus_master of bridge device.

It also doesn't *enable* it though, does it?

johannes

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-14 17:44                   ` Jens Axboe
@ 2017-09-14 20:04                     ` Srinath Mannam
  2017-09-14 20:36                       ` Jens Axboe
  0 siblings, 1 reply; 31+ messages in thread
From: Srinath Mannam @ 2017-09-14 20:04 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Bjorn Helgaas, Johannes Berg, Luca Coelho, Grumbach, Emmanuel,
	linuxwifi, linux-wireless, linux-pci

Hi Jens Axboe,


On Thu, Sep 14, 2017 at 11:14 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 09/14/2017 11:35 AM, Jens Axboe wrote:
>> On 09/14/2017 11:28 AM, Srinath Mannam wrote:
>>> Hi Bjorn,
>>>
>>> On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>
>>>> On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
>>>>> [+cc linux-pci]
>>>>>
>>>>> On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>>> On 09/12/2017 02:04 PM, Johannes Berg wrote:
>>>>>>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>>>>>>>
>>>>>>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>>>>>>>> pci_is_enabled() check, since the rest of the patch shouldn't have
>>>>>>>> functional changes.
>>>>>>>
>>>>>>> and pci_enable_bridge() already checks if it's already enabled, but
>>>>>>> still enables mastering in that case if it isn't:
>>>>>>>
>>>>>>> static void pci_enable_bridge(struct pci_dev *dev)
>>>>>>> {
>>>>>>> [...]
>>>>>>>         if (pci_is_enabled(dev)) {
>>>>>>>                 if (!dev->is_busmaster)
>>>>>>>                         pci_set_master(dev);
>>>>>>>                 return;
>>>>>>>         }
>>>>>>>
>>>>>>> so I guess due to the new check we end up with mastering disabled, and
>>>>>>> thus the firmware can't load since that's a DMA thing?
>>>>>>
>>>>>> Bjorn/Srinath, any input here? This is a regression that prevents wifi
>>>>>> from working on a pretty standard laptop. It'd suck to have this be in
>>>>>> -rc1. Seems like the trivial fix would be:
>>>>>>
>>>>>>
>>>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>>>> index b0002daa50f3..ffbe11dbdd61 100644
>>>>>> --- a/drivers/pci/pci.c
>>>>>> +++ b/drivers/pci/pci.c
>>>>>> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>>>>>>                 return 0;               /* already enabled */
>>>>>>
>>>>>>         bridge = pci_upstream_bridge(dev);
>>>>>> -       if (bridge && !pci_is_enabled(bridge))
>>>>>> +       if (bridge)
>>> With this change and keeping "mutex_lock(&pci_bridge_mutex);" in
>>> pci_enable_bridge functoin will causes a nexted lock.
>>
>> Took a look, and looks like you are right. That code looks like a mess,
>> fwiw.
>>
>> I'd strongly suggest that the bad commit is reverted until a proper
>> solution is found, since the simple one-liner could potentially
>> introduce a deadlock with your patch applied.
>
> BTW, your patch looks pretty bad too, introducing a random mutex
> deep on code that can be recursive. Why isn't this check in
> pci_enable_device_flags() enough to prevent double-enable of
> an upstream bridge?
>
> if (atomic_inc_return(&dev->enable_cnt) > 1)
>         return 0;               /* already enabled */
>
This check only to verify device enable not for the bus master check.
But device enable function calls the bridge enable if it has the bridge.
Bridge enable function enables both device and bus master.

Here the issue might be because, bridge of endpoint has already set
device enable without set bus master in some other context. which is
wrong.
because all bridges should enable with bridge_enable function only.
So we see the problem In this context, because "if (bridge &&
!pci_is_enabled(bridge))" check stops bridge enable which intern stops
bus master.
pci_enable_bridge function always makes sure that both device and bus
master are enabled in any case. If pci_enable_bridge function is not
called means, that bridge is already has device enable flag set. which
is not from pci_enable_bridge function.

Regards,
Srinath.

> --
> Jens Axboe
>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-14 20:04                     ` Srinath Mannam
@ 2017-09-14 20:36                       ` Jens Axboe
  2017-09-15 19:32                         ` Jens Axboe
  0 siblings, 1 reply; 31+ messages in thread
From: Jens Axboe @ 2017-09-14 20:36 UTC (permalink / raw)
  To: Srinath Mannam
  Cc: Bjorn Helgaas, Johannes Berg, Luca Coelho, Grumbach, Emmanuel,
	linuxwifi, linux-wireless, linux-pci

On 09/14/2017 02:04 PM, Srinath Mannam wrote:
> Hi Jens Axboe,
> 
> 
> On Thu, Sep 14, 2017 at 11:14 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 09/14/2017 11:35 AM, Jens Axboe wrote:
>>> On 09/14/2017 11:28 AM, Srinath Mannam wrote:
>>>> Hi Bjorn,
>>>>
>>>> On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>>
>>>>> On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
>>>>>> [+cc linux-pci]
>>>>>>
>>>>>> On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>> On 09/12/2017 02:04 PM, Johannes Berg wrote:
>>>>>>>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>>>>>>>>
>>>>>>>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>>>>>>>>> pci_is_enabled() check, since the rest of the patch shouldn't have
>>>>>>>>> functional changes.
>>>>>>>>
>>>>>>>> and pci_enable_bridge() already checks if it's already enabled, but
>>>>>>>> still enables mastering in that case if it isn't:
>>>>>>>>
>>>>>>>> static void pci_enable_bridge(struct pci_dev *dev)
>>>>>>>> {
>>>>>>>> [...]
>>>>>>>>         if (pci_is_enabled(dev)) {
>>>>>>>>                 if (!dev->is_busmaster)
>>>>>>>>                         pci_set_master(dev);
>>>>>>>>                 return;
>>>>>>>>         }
>>>>>>>>
>>>>>>>> so I guess due to the new check we end up with mastering disabled, and
>>>>>>>> thus the firmware can't load since that's a DMA thing?
>>>>>>>
>>>>>>> Bjorn/Srinath, any input here? This is a regression that prevents wifi
>>>>>>> from working on a pretty standard laptop. It'd suck to have this be in
>>>>>>> -rc1. Seems like the trivial fix would be:
>>>>>>>
>>>>>>>
>>>>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>>>>> index b0002daa50f3..ffbe11dbdd61 100644
>>>>>>> --- a/drivers/pci/pci.c
>>>>>>> +++ b/drivers/pci/pci.c
>>>>>>> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>>>>>>>                 return 0;               /* already enabled */
>>>>>>>
>>>>>>>         bridge = pci_upstream_bridge(dev);
>>>>>>> -       if (bridge && !pci_is_enabled(bridge))
>>>>>>> +       if (bridge)
>>>> With this change and keeping "mutex_lock(&pci_bridge_mutex);" in
>>>> pci_enable_bridge functoin will causes a nexted lock.
>>>
>>> Took a look, and looks like you are right. That code looks like a mess,
>>> fwiw.
>>>
>>> I'd strongly suggest that the bad commit is reverted until a proper
>>> solution is found, since the simple one-liner could potentially
>>> introduce a deadlock with your patch applied.
>>
>> BTW, your patch looks pretty bad too, introducing a random mutex
>> deep on code that can be recursive. Why isn't this check in
>> pci_enable_device_flags() enough to prevent double-enable of
>> an upstream bridge?
>>
>> if (atomic_inc_return(&dev->enable_cnt) > 1)
>>         return 0;               /* already enabled */
>>
> This check only to verify device enable not for the bus master check.
> But device enable function calls the bridge enable if it has the bridge.
> Bridge enable function enables both device and bus master.
> 
> Here the issue might be because, bridge of endpoint has already set
> device enable without set bus master in some other context. which is
> wrong.
> because all bridges should enable with bridge_enable function only.
> So we see the problem In this context, because "if (bridge &&
> !pci_is_enabled(bridge))" check stops bridge enable which intern stops
> bus master.
> pci_enable_bridge function always makes sure that both device and bus
> master are enabled in any case. If pci_enable_bridge function is not
> called means, that bridge is already has device enable flag set. which
> is not from pci_enable_bridge function.

In any case, your patch introduces a regression on systems. Please get
it reverted now, and then you can come up with a new approach to fix the
double enable of the upstream bridge.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-14 20:36                       ` Jens Axboe
@ 2017-09-15 19:32                         ` Jens Axboe
  2017-09-15 19:36                           ` Luca Coelho
  2017-09-15 19:38                           ` Linus Torvalds
  0 siblings, 2 replies; 31+ messages in thread
From: Jens Axboe @ 2017-09-15 19:32 UTC (permalink / raw)
  To: Srinath Mannam
  Cc: Bjorn Helgaas, Johannes Berg, Luca Coelho, Grumbach, Emmanuel,
	linuxwifi, linux-wireless, linux-pci, Linus Torvalds

On 09/14/2017 02:36 PM, Jens Axboe wrote:
> On 09/14/2017 02:04 PM, Srinath Mannam wrote:
>> Hi Jens Axboe,
>>
>>
>> On Thu, Sep 14, 2017 at 11:14 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>> On 09/14/2017 11:35 AM, Jens Axboe wrote:
>>>> On 09/14/2017 11:28 AM, Srinath Mannam wrote:
>>>>> Hi Bjorn,
>>>>>
>>>>> On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>
>>>>>> On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
>>>>>>> [+cc linux-pci]
>>>>>>>
>>>>>>> On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>>> On 09/12/2017 02:04 PM, Johannes Berg wrote:
>>>>>>>>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>>>>>>>>>
>>>>>>>>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>>>>>>>>>> pci_is_enabled() check, since the rest of the patch shouldn't have
>>>>>>>>>> functional changes.
>>>>>>>>>
>>>>>>>>> and pci_enable_bridge() already checks if it's already enabled, but
>>>>>>>>> still enables mastering in that case if it isn't:
>>>>>>>>>
>>>>>>>>> static void pci_enable_bridge(struct pci_dev *dev)
>>>>>>>>> {
>>>>>>>>> [...]
>>>>>>>>>         if (pci_is_enabled(dev)) {
>>>>>>>>>                 if (!dev->is_busmaster)
>>>>>>>>>                         pci_set_master(dev);
>>>>>>>>>                 return;
>>>>>>>>>         }
>>>>>>>>>
>>>>>>>>> so I guess due to the new check we end up with mastering disabled, and
>>>>>>>>> thus the firmware can't load since that's a DMA thing?
>>>>>>>>
>>>>>>>> Bjorn/Srinath, any input here? This is a regression that prevents wifi
>>>>>>>> from working on a pretty standard laptop. It'd suck to have this be in
>>>>>>>> -rc1. Seems like the trivial fix would be:
>>>>>>>>
>>>>>>>>
>>>>>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>>>>>> index b0002daa50f3..ffbe11dbdd61 100644
>>>>>>>> --- a/drivers/pci/pci.c
>>>>>>>> +++ b/drivers/pci/pci.c
>>>>>>>> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>>>>>>>>                 return 0;               /* already enabled */
>>>>>>>>
>>>>>>>>         bridge = pci_upstream_bridge(dev);
>>>>>>>> -       if (bridge && !pci_is_enabled(bridge))
>>>>>>>> +       if (bridge)
>>>>> With this change and keeping "mutex_lock(&pci_bridge_mutex);" in
>>>>> pci_enable_bridge functoin will causes a nexted lock.
>>>>
>>>> Took a look, and looks like you are right. That code looks like a mess,
>>>> fwiw.
>>>>
>>>> I'd strongly suggest that the bad commit is reverted until a proper
>>>> solution is found, since the simple one-liner could potentially
>>>> introduce a deadlock with your patch applied.
>>>
>>> BTW, your patch looks pretty bad too, introducing a random mutex
>>> deep on code that can be recursive. Why isn't this check in
>>> pci_enable_device_flags() enough to prevent double-enable of
>>> an upstream bridge?
>>>
>>> if (atomic_inc_return(&dev->enable_cnt) > 1)
>>>         return 0;               /* already enabled */
>>>
>> This check only to verify device enable not for the bus master check.
>> But device enable function calls the bridge enable if it has the bridge.
>> Bridge enable function enables both device and bus master.
>>
>> Here the issue might be because, bridge of endpoint has already set
>> device enable without set bus master in some other context. which is
>> wrong.
>> because all bridges should enable with bridge_enable function only.
>> So we see the problem In this context, because "if (bridge &&
>> !pci_is_enabled(bridge))" check stops bridge enable which intern stops
>> bus master.
>> pci_enable_bridge function always makes sure that both device and bus
>> master are enabled in any case. If pci_enable_bridge function is not
>> called means, that bridge is already has device enable flag set. which
>> is not from pci_enable_bridge function.
> 
> In any case, your patch introduces a regression on systems. Please get
> it reverted now, and then you can come up with a new approach to fix the
> double enable of the upstream bridge.

Who's sending in the revert? I can certainly do it if no one else does,
but it needs to be done.

I'm not seeing any patches coming out of Srinath to fix up the
situation, so we should revert the broken patch until a better solution
exists.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-15 19:32                         ` Jens Axboe
@ 2017-09-15 19:36                           ` Luca Coelho
  2017-09-15 19:46                             ` Jens Axboe
  2017-09-15 19:38                           ` Linus Torvalds
  1 sibling, 1 reply; 31+ messages in thread
From: Luca Coelho @ 2017-09-15 19:36 UTC (permalink / raw)
  To: Jens Axboe, Srinath Mannam
  Cc: Bjorn Helgaas, Johannes Berg, Grumbach, Emmanuel, linuxwifi,
	linux-wireless, linux-pci, Linus Torvalds

On Fri, 2017-09-15 at 13:32 -0600, Jens Axboe wrote:
> On 09/14/2017 02:36 PM, Jens Axboe wrote:
> > On 09/14/2017 02:04 PM, Srinath Mannam wrote:
> > > Hi Jens Axboe,
> > > 
> > > 
> > > On Thu, Sep 14, 2017 at 11:14 PM, Jens Axboe <axboe@kernel.dk> wrote:
> > > > On 09/14/2017 11:35 AM, Jens Axboe wrote:
> > > > > On 09/14/2017 11:28 AM, Srinath Mannam wrote:
> > > > > > Hi Bjorn,
> > > > > > 
> > > > > > On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe <axboe@kernel.dk> wrote:
> > > > > > > 
> > > > > > > On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
> > > > > > > > [+cc linux-pci]
> > > > > > > > 
> > > > > > > > On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
> > > > > > > > > On 09/12/2017 02:04 PM, Johannes Berg wrote:
> > > > > > > > > > On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
> > > > > > > > > > 
> > > > > > > > > > > CC'ing the guilty part and Bjorn. I'm assuming it's the
> > > > > > > > > > > pci_is_enabled() check, since the rest of the patch shouldn't have
> > > > > > > > > > > functional changes.
> > > > > > > > > > 
> > > > > > > > > > and pci_enable_bridge() already checks if it's already enabled, but
> > > > > > > > > > still enables mastering in that case if it isn't:
> > > > > > > > > > 
> > > > > > > > > > static void pci_enable_bridge(struct pci_dev *dev)
> > > > > > > > > > {
> > > > > > > > > > [...]
> > > > > > > > > >         if (pci_is_enabled(dev)) {
> > > > > > > > > >                 if (!dev->is_busmaster)
> > > > > > > > > >                         pci_set_master(dev);
> > > > > > > > > >                 return;
> > > > > > > > > >         }
> > > > > > > > > > 
> > > > > > > > > > so I guess due to the new check we end up with mastering disabled, and
> > > > > > > > > > thus the firmware can't load since that's a DMA thing?
> > > > > > > > > 
> > > > > > > > > Bjorn/Srinath, any input here? This is a regression that prevents wifi
> > > > > > > > > from working on a pretty standard laptop. It'd suck to have this be in
> > > > > > > > > -rc1. Seems like the trivial fix would be:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > > > > > index b0002daa50f3..ffbe11dbdd61 100644
> > > > > > > > > --- a/drivers/pci/pci.c
> > > > > > > > > +++ b/drivers/pci/pci.c
> > > > > > > > > @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
> > > > > > > > >                 return 0;               /* already enabled */
> > > > > > > > > 
> > > > > > > > >         bridge = pci_upstream_bridge(dev);
> > > > > > > > > -       if (bridge && !pci_is_enabled(bridge))
> > > > > > > > > +       if (bridge)
> > > > > > 
> > > > > > With this change and keeping "mutex_lock(&pci_bridge_mutex);" in
> > > > > > pci_enable_bridge functoin will causes a nexted lock.
> > > > > 
> > > > > Took a look, and looks like you are right. That code looks like a mess,
> > > > > fwiw.
> > > > > 
> > > > > I'd strongly suggest that the bad commit is reverted until a proper
> > > > > solution is found, since the simple one-liner could potentially
> > > > > introduce a deadlock with your patch applied.
> > > > 
> > > > BTW, your patch looks pretty bad too, introducing a random mutex
> > > > deep on code that can be recursive. Why isn't this check in
> > > > pci_enable_device_flags() enough to prevent double-enable of
> > > > an upstream bridge?
> > > > 
> > > > if (atomic_inc_return(&dev->enable_cnt) > 1)
> > > >         return 0;               /* already enabled */
> > > > 
> > > 
> > > This check only to verify device enable not for the bus master check.
> > > But device enable function calls the bridge enable if it has the bridge.
> > > Bridge enable function enables both device and bus master.
> > > 
> > > Here the issue might be because, bridge of endpoint has already set
> > > device enable without set bus master in some other context. which is
> > > wrong.
> > > because all bridges should enable with bridge_enable function only.
> > > So we see the problem In this context, because "if (bridge &&
> > > !pci_is_enabled(bridge))" check stops bridge enable which intern stops
> > > bus master.
> > > pci_enable_bridge function always makes sure that both device and bus
> > > master are enabled in any case. If pci_enable_bridge function is not
> > > called means, that bridge is already has device enable flag set. which
> > > is not from pci_enable_bridge function.
> > 
> > In any case, your patch introduces a regression on systems. Please get
> > it reverted now, and then you can come up with a new approach to fix the
> > double enable of the upstream bridge.
> 
> Who's sending in the revert? I can certainly do it if no one else does,
> but it needs to be done.
> 
> I'm not seeing any patches coming out of Srinath to fix up the
> situation, so we should revert the broken patch until a better solution
> exists.

Bjorn already sent it:

https://lkml.org/lkml/2017/9/15/46

--
Cheers,
Luca.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-15 19:32                         ` Jens Axboe
  2017-09-15 19:36                           ` Luca Coelho
@ 2017-09-15 19:38                           ` Linus Torvalds
  2017-09-15 19:43                             ` Luca Coelho
  2017-09-15 19:48                             ` Jens Axboe
  1 sibling, 2 replies; 31+ messages in thread
From: Linus Torvalds @ 2017-09-15 19:38 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Srinath Mannam, Bjorn Helgaas, Johannes Berg, Luca Coelho,
	Grumbach, Emmanuel, linuxwifi, linux-wireless, linux-pci

On Fri, Sep 15, 2017 at 12:32 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>
>> In any case, your patch introduces a regression on systems. Please get
>> it reverted now, and then you can come up with a new approach to fix the
>> double enable of the upstream bridge.
>
> Who's sending in the revert? I can certainly do it if no one else does,
> but it needs to be done.
>
> I'm not seeing any patches coming out of Srinath to fix up the
> situation, so we should revert the broken patch until a better solution
> exists.

Hmm. I don't have the history here (apparently it never made lkml, for
example), so I don't even know which commit you're talking about.

>From some of the context it looks like commit 40f11adc7cd9 ("PCI:
Avoid race while enabling upstream bridges"), is that correct?

           Linus

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-15 19:38                           ` Linus Torvalds
@ 2017-09-15 19:43                             ` Luca Coelho
  2017-09-15 19:51                               ` Linus Torvalds
  2017-09-15 19:48                             ` Jens Axboe
  1 sibling, 1 reply; 31+ messages in thread
From: Luca Coelho @ 2017-09-15 19:43 UTC (permalink / raw)
  To: Linus Torvalds, Jens Axboe
  Cc: Srinath Mannam, Bjorn Helgaas, Johannes Berg, Grumbach, Emmanuel,
	linuxwifi, linux-wireless, linux-pci

On Fri, 2017-09-15 at 12:38 -0700, Linus Torvalds wrote:
> On Fri, Sep 15, 2017 at 12:32 PM, Jens Axboe <axboe@kernel.dk> wrote:
> > > 
> > > In any case, your patch introduces a regression on systems. Please get
> > > it reverted now, and then you can come up with a new approach to fix the
> > > double enable of the upstream bridge.
> > 
> > Who's sending in the revert? I can certainly do it if no one else does,
> > but it needs to be done.
> > 
> > I'm not seeing any patches coming out of Srinath to fix up the
> > situation, so we should revert the broken patch until a better solution
> > exists.
> 
> Hmm. I don't have the history here (apparently it never made lkml, for
> example), so I don't even know which commit you're talking about.
> 
> From some of the context it looks like commit 40f11adc7cd9 ("PCI:
> Avoid race while enabling upstream bridges"), is that correct?

Yes, that's the one.  And Bjorn already sent a revert:

https://lkml.org/lkml/2017/9/15/46

--
Luca.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-15 19:36                           ` Luca Coelho
@ 2017-09-15 19:46                             ` Jens Axboe
  0 siblings, 0 replies; 31+ messages in thread
From: Jens Axboe @ 2017-09-15 19:46 UTC (permalink / raw)
  To: Luca Coelho, Srinath Mannam
  Cc: Bjorn Helgaas, Johannes Berg, Grumbach, Emmanuel, linuxwifi,
	linux-wireless, linux-pci, Linus Torvalds

On 09/15/2017 01:36 PM, Luca Coelho wrote:
> On Fri, 2017-09-15 at 13:32 -0600, Jens Axboe wrote:
>> On 09/14/2017 02:36 PM, Jens Axboe wrote:
>>> On 09/14/2017 02:04 PM, Srinath Mannam wrote:
>>>> Hi Jens Axboe,
>>>>
>>>>
>>>> On Thu, Sep 14, 2017 at 11:14 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>> On 09/14/2017 11:35 AM, Jens Axboe wrote:
>>>>>> On 09/14/2017 11:28 AM, Srinath Mannam wrote:
>>>>>>> Hi Bjorn,
>>>>>>>
>>>>>>> On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>>>
>>>>>>>> On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
>>>>>>>>> [+cc linux-pci]
>>>>>>>>>
>>>>>>>>> On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>>>>> On 09/12/2017 02:04 PM, Johannes Berg wrote:
>>>>>>>>>>> On Tue, 2017-09-12 at 13:43 -0600, Jens Axboe wrote:
>>>>>>>>>>>
>>>>>>>>>>>> CC'ing the guilty part and Bjorn. I'm assuming it's the
>>>>>>>>>>>> pci_is_enabled() check, since the rest of the patch shouldn't have
>>>>>>>>>>>> functional changes.
>>>>>>>>>>>
>>>>>>>>>>> and pci_enable_bridge() already checks if it's already enabled, but
>>>>>>>>>>> still enables mastering in that case if it isn't:
>>>>>>>>>>>
>>>>>>>>>>> static void pci_enable_bridge(struct pci_dev *dev)
>>>>>>>>>>> {
>>>>>>>>>>> [...]
>>>>>>>>>>>         if (pci_is_enabled(dev)) {
>>>>>>>>>>>                 if (!dev->is_busmaster)
>>>>>>>>>>>                         pci_set_master(dev);
>>>>>>>>>>>                 return;
>>>>>>>>>>>         }
>>>>>>>>>>>
>>>>>>>>>>> so I guess due to the new check we end up with mastering disabled, and
>>>>>>>>>>> thus the firmware can't load since that's a DMA thing?
>>>>>>>>>>
>>>>>>>>>> Bjorn/Srinath, any input here? This is a regression that prevents wifi
>>>>>>>>>> from working on a pretty standard laptop. It'd suck to have this be in
>>>>>>>>>> -rc1. Seems like the trivial fix would be:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>>>>>>>> index b0002daa50f3..ffbe11dbdd61 100644
>>>>>>>>>> --- a/drivers/pci/pci.c
>>>>>>>>>> +++ b/drivers/pci/pci.c
>>>>>>>>>> @@ -1394,7 +1394,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>>>>>>>>>>                 return 0;               /* already enabled */
>>>>>>>>>>
>>>>>>>>>>         bridge = pci_upstream_bridge(dev);
>>>>>>>>>> -       if (bridge && !pci_is_enabled(bridge))
>>>>>>>>>> +       if (bridge)
>>>>>>>
>>>>>>> With this change and keeping "mutex_lock(&pci_bridge_mutex);" in
>>>>>>> pci_enable_bridge functoin will causes a nexted lock.
>>>>>>
>>>>>> Took a look, and looks like you are right. That code looks like a mess,
>>>>>> fwiw.
>>>>>>
>>>>>> I'd strongly suggest that the bad commit is reverted until a proper
>>>>>> solution is found, since the simple one-liner could potentially
>>>>>> introduce a deadlock with your patch applied.
>>>>>
>>>>> BTW, your patch looks pretty bad too, introducing a random mutex
>>>>> deep on code that can be recursive. Why isn't this check in
>>>>> pci_enable_device_flags() enough to prevent double-enable of
>>>>> an upstream bridge?
>>>>>
>>>>> if (atomic_inc_return(&dev->enable_cnt) > 1)
>>>>>         return 0;               /* already enabled */
>>>>>
>>>>
>>>> This check only to verify device enable not for the bus master check.
>>>> But device enable function calls the bridge enable if it has the bridge.
>>>> Bridge enable function enables both device and bus master.
>>>>
>>>> Here the issue might be because, bridge of endpoint has already set
>>>> device enable without set bus master in some other context. which is
>>>> wrong.
>>>> because all bridges should enable with bridge_enable function only.
>>>> So we see the problem In this context, because "if (bridge &&
>>>> !pci_is_enabled(bridge))" check stops bridge enable which intern stops
>>>> bus master.
>>>> pci_enable_bridge function always makes sure that both device and bus
>>>> master are enabled in any case. If pci_enable_bridge function is not
>>>> called means, that bridge is already has device enable flag set. which
>>>> is not from pci_enable_bridge function.
>>>
>>> In any case, your patch introduces a regression on systems. Please get
>>> it reverted now, and then you can come up with a new approach to fix the
>>> double enable of the upstream bridge.
>>
>> Who's sending in the revert? I can certainly do it if no one else does,
>> but it needs to be done.
>>
>> I'm not seeing any patches coming out of Srinath to fix up the
>> situation, so we should revert the broken patch until a better solution
>> exists.
> 
> Bjorn already sent it:
> 
> https://lkml.org/lkml/2017/9/15/46

Huh ok, I would have thought the various folks in this discussion would
have been CC'ed on that. But good to know, that takes care of my
concern.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-15 19:38                           ` Linus Torvalds
  2017-09-15 19:43                             ` Luca Coelho
@ 2017-09-15 19:48                             ` Jens Axboe
  2017-09-15 19:51                               ` Luca Coelho
  1 sibling, 1 reply; 31+ messages in thread
From: Jens Axboe @ 2017-09-15 19:48 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Srinath Mannam, Bjorn Helgaas, Johannes Berg, Luca Coelho,
	Grumbach, Emmanuel, linuxwifi, linux-wireless, linux-pci

On 09/15/2017 01:38 PM, Linus Torvalds wrote:
> On Fri, Sep 15, 2017 at 12:32 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> In any case, your patch introduces a regression on systems. Please get
>>> it reverted now, and then you can come up with a new approach to fix the
>>> double enable of the upstream bridge.
>>
>> Who's sending in the revert? I can certainly do it if no one else does,
>> but it needs to be done.
>>
>> I'm not seeing any patches coming out of Srinath to fix up the
>> situation, so we should revert the broken patch until a better solution
>> exists.
> 
> Hmm. I don't have the history here (apparently it never made lkml, for
> example), so I don't even know which commit you're talking about.
> 
> From some of the context it looks like commit 40f11adc7cd9 ("PCI:
> Avoid race while enabling upstream bridges"), is that correct?

Yes, Luca says that Bjorn already sent in the revert request, I just
didn't see it since I wasn't CC'ed on it. So looks like we're all
good, provided that makes it into -rc1. 40f11adc7cd9 is the broken
commit.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-15 19:48                             ` Jens Axboe
@ 2017-09-15 19:51                               ` Luca Coelho
  2017-09-15 19:55                                 ` Jens Axboe
  0 siblings, 1 reply; 31+ messages in thread
From: Luca Coelho @ 2017-09-15 19:51 UTC (permalink / raw)
  To: Jens Axboe, Linus Torvalds
  Cc: Srinath Mannam, Bjorn Helgaas, Johannes Berg, Grumbach, Emmanuel,
	linuxwifi, linux-wireless, linux-pci

On Fri, 2017-09-15 at 13:48 -0600, Jens Axboe wrote:
> On 09/15/2017 01:38 PM, Linus Torvalds wrote:
> > On Fri, Sep 15, 2017 at 12:32 PM, Jens Axboe <axboe@kernel.dk> wrote:
> > > > 
> > > > In any case, your patch introduces a regression on systems. Please get
> > > > it reverted now, and then you can come up with a new approach to fix the
> > > > double enable of the upstream bridge.
> > > 
> > > Who's sending in the revert? I can certainly do it if no one else does,
> > > but it needs to be done.
> > > 
> > > I'm not seeing any patches coming out of Srinath to fix up the
> > > situation, so we should revert the broken patch until a better solution
> > > exists.
> > 
> > Hmm. I don't have the history here (apparently it never made lkml, for
> > example), so I don't even know which commit you're talking about.
> > 
> > From some of the context it looks like commit 40f11adc7cd9 ("PCI:
> > Avoid race while enabling upstream bridges"), is that correct?
> 
> Yes, Luca says that Bjorn already sent in the revert request, I just
> didn't see it since I wasn't CC'ed on it. So looks like we're all
> good, provided that makes it into -rc1. 40f11adc7cd9 is the broken
> commit.

Strange... AFAICT you *were* CCed on it.  And so was everyone else in
the original thread (+LKML)...

--
Luca.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-15 19:43                             ` Luca Coelho
@ 2017-09-15 19:51                               ` Linus Torvalds
  2017-09-15 19:57                                 ` Jens Axboe
  0 siblings, 1 reply; 31+ messages in thread
From: Linus Torvalds @ 2017-09-15 19:51 UTC (permalink / raw)
  To: Luca Coelho
  Cc: Jens Axboe, Srinath Mannam, Bjorn Helgaas, Johannes Berg,
	Grumbach, Emmanuel, linuxwifi, linux-wireless, linux-pci

On Fri, Sep 15, 2017 at 12:43 PM, Luca Coelho <luca@coelho.fi> wrote:
> On Fri, 2017-09-15 at 12:38 -0700, Linus Torvalds wrote:
>>
>> From some of the context it looks like commit 40f11adc7cd9 ("PCI:
>> Avoid race while enabling upstream bridges"), is that correct?
>
> Yes, that's the one.  And Bjorn already sent a revert:
>
> https://lkml.org/lkml/2017/9/15/46

Well, he may have sent a revert to lkml, but not to me. I'm assuming
it's in his tree and I'll get a pull request. Hopefully soon, so that
it makes rc.

Jens, you were actually cc'd on that revert according to the email
headers, so check your spam-box.

                 Linus

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-15 19:51                               ` Luca Coelho
@ 2017-09-15 19:55                                 ` Jens Axboe
  2017-09-16  3:03                                   ` Bjorn Helgaas
  0 siblings, 1 reply; 31+ messages in thread
From: Jens Axboe @ 2017-09-15 19:55 UTC (permalink / raw)
  To: Luca Coelho, Linus Torvalds
  Cc: Srinath Mannam, Bjorn Helgaas, Johannes Berg, Grumbach, Emmanuel,
	linuxwifi, linux-wireless, linux-pci

On 09/15/2017 01:51 PM, Luca Coelho wrote:
> On Fri, 2017-09-15 at 13:48 -0600, Jens Axboe wrote:
>> On 09/15/2017 01:38 PM, Linus Torvalds wrote:
>>> On Fri, Sep 15, 2017 at 12:32 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>>
>>>>> In any case, your patch introduces a regression on systems. Please get
>>>>> it reverted now, and then you can come up with a new approach to fix the
>>>>> double enable of the upstream bridge.
>>>>
>>>> Who's sending in the revert? I can certainly do it if no one else does,
>>>> but it needs to be done.
>>>>
>>>> I'm not seeing any patches coming out of Srinath to fix up the
>>>> situation, so we should revert the broken patch until a better solution
>>>> exists.
>>>
>>> Hmm. I don't have the history here (apparently it never made lkml, for
>>> example), so I don't even know which commit you're talking about.
>>>
>>> From some of the context it looks like commit 40f11adc7cd9 ("PCI:
>>> Avoid race while enabling upstream bridges"), is that correct?
>>
>> Yes, Luca says that Bjorn already sent in the revert request, I just
>> didn't see it since I wasn't CC'ed on it. So looks like we're all
>> good, provided that makes it into -rc1. 40f11adc7cd9 is the broken
>> commit.
> 
> Strange... AFAICT you *were* CCed on it.  And so was everyone else in
> the original thread (+LKML)...

Hmm, never showed up here. Very odd!

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-15 19:51                               ` Linus Torvalds
@ 2017-09-15 19:57                                 ` Jens Axboe
  0 siblings, 0 replies; 31+ messages in thread
From: Jens Axboe @ 2017-09-15 19:57 UTC (permalink / raw)
  To: Linus Torvalds, Luca Coelho
  Cc: Srinath Mannam, Bjorn Helgaas, Johannes Berg, Grumbach, Emmanuel,
	linuxwifi, linux-wireless, linux-pci

On 09/15/2017 01:51 PM, Linus Torvalds wrote:
> On Fri, Sep 15, 2017 at 12:43 PM, Luca Coelho <luca@coelho.fi> wrote:
>> On Fri, 2017-09-15 at 12:38 -0700, Linus Torvalds wrote:
>>>
>>> From some of the context it looks like commit 40f11adc7cd9 ("PCI:
>>> Avoid race while enabling upstream bridges"), is that correct?
>>
>> Yes, that's the one.  And Bjorn already sent a revert:
>>
>> https://lkml.org/lkml/2017/9/15/46
> 
> Well, he may have sent a revert to lkml, but not to me. I'm assuming
> it's in his tree and I'll get a pull request. Hopefully soon, so that
> it makes rc.

That was my hope, and why I emailed again today on the topic.

> Jens, you were actually cc'd on that revert according to the email
> headers, so check your spam-box.

Yeah, Luca says so too. Which is making me a little worried on behalf
of my email, since it's not sitting in spam.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-15 19:55                                 ` Jens Axboe
@ 2017-09-16  3:03                                   ` Bjorn Helgaas
  2017-09-16 18:53                                     ` Jens Axboe
  0 siblings, 1 reply; 31+ messages in thread
From: Bjorn Helgaas @ 2017-09-16  3:03 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Luca Coelho, Linus Torvalds, Srinath Mannam, Bjorn Helgaas,
	Johannes Berg, Grumbach, Emmanuel, linuxwifi, linux-wireless,
	linux-pci

On Fri, Sep 15, 2017 at 01:55:57PM -0600, Jens Axboe wrote:
> On 09/15/2017 01:51 PM, Luca Coelho wrote:
> > On Fri, 2017-09-15 at 13:48 -0600, Jens Axboe wrote:
> >> On 09/15/2017 01:38 PM, Linus Torvalds wrote:
> >>> On Fri, Sep 15, 2017 at 12:32 PM, Jens Axboe <axboe@kernel.dk> wrote:
> >>>>>
> >>>>> In any case, your patch introduces a regression on systems. Please get
> >>>>> it reverted now, and then you can come up with a new approach to fix the
> >>>>> double enable of the upstream bridge.
> >>>>
> >>>> Who's sending in the revert? I can certainly do it if no one else does,
> >>>> but it needs to be done.
> >>>>
> >>>> I'm not seeing any patches coming out of Srinath to fix up the
> >>>> situation, so we should revert the broken patch until a better solution
> >>>> exists.
> >>>
> >>> Hmm. I don't have the history here (apparently it never made lkml, for
> >>> example), so I don't even know which commit you're talking about.
> >>>
> >>> From some of the context it looks like commit 40f11adc7cd9 ("PCI:
> >>> Avoid race while enabling upstream bridges"), is that correct?
> >>
> >> Yes, Luca says that Bjorn already sent in the revert request, I just
> >> didn't see it since I wasn't CC'ed on it. So looks like we're all
> >> good, provided that makes it into -rc1. 40f11adc7cd9 is the broken
> >> commit.
> > 
> > Strange... AFAICT you *were* CCed on it.  And so was everyone else in
> > the original thread (+LKML)...
> 
> Hmm, never showed up here. Very odd!

Sorry, I think this is probably because I'm an idiot and sent it from
an @google.com account and it got rejected because the DMARC check
failed.

Bjorn

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: iwlwifi firmware load broken in current -git
  2017-09-16  3:03                                   ` Bjorn Helgaas
@ 2017-09-16 18:53                                     ` Jens Axboe
  0 siblings, 0 replies; 31+ messages in thread
From: Jens Axboe @ 2017-09-16 18:53 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Luca Coelho, Linus Torvalds, Srinath Mannam, Bjorn Helgaas,
	Johannes Berg, Grumbach, Emmanuel, linuxwifi, linux-wireless,
	linux-pci

On 09/15/2017 09:03 PM, Bjorn Helgaas wrote:
> On Fri, Sep 15, 2017 at 01:55:57PM -0600, Jens Axboe wrote:
>> On 09/15/2017 01:51 PM, Luca Coelho wrote:
>>> On Fri, 2017-09-15 at 13:48 -0600, Jens Axboe wrote:
>>>> On 09/15/2017 01:38 PM, Linus Torvalds wrote:
>>>>> On Fri, Sep 15, 2017 at 12:32 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>>>>
>>>>>>> In any case, your patch introduces a regression on systems. Please get
>>>>>>> it reverted now, and then you can come up with a new approach to fix the
>>>>>>> double enable of the upstream bridge.
>>>>>>
>>>>>> Who's sending in the revert? I can certainly do it if no one else does,
>>>>>> but it needs to be done.
>>>>>>
>>>>>> I'm not seeing any patches coming out of Srinath to fix up the
>>>>>> situation, so we should revert the broken patch until a better solution
>>>>>> exists.
>>>>>
>>>>> Hmm. I don't have the history here (apparently it never made lkml, for
>>>>> example), so I don't even know which commit you're talking about.
>>>>>
>>>>> From some of the context it looks like commit 40f11adc7cd9 ("PCI:
>>>>> Avoid race while enabling upstream bridges"), is that correct?
>>>>
>>>> Yes, Luca says that Bjorn already sent in the revert request, I just
>>>> didn't see it since I wasn't CC'ed on it. So looks like we're all
>>>> good, provided that makes it into -rc1. 40f11adc7cd9 is the broken
>>>> commit.
>>>
>>> Strange... AFAICT you *were* CCed on it.  And so was everyone else in
>>> the original thread (+LKML)...
>>
>> Hmm, never showed up here. Very odd!
> 
> Sorry, I think this is probably because I'm an idiot and sent it from
> an @google.com account and it got rejected because the DMARC check
> failed.

Ah, good to know why it didn't show up. Thanks.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2017-09-16 18:53 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-12 15:48 iwlwifi firmware load broken in current -git Jens Axboe
2017-09-12 16:11 ` Coelho, Luciano
2017-09-12 16:35   ` Jens Axboe
2017-09-12 16:36   ` Luca Coelho
2017-09-12 16:59     ` Jens Axboe
2017-09-12 19:43     ` Jens Axboe
2017-09-12 19:51       ` Luca Coelho
2017-09-12 20:04       ` Johannes Berg
2017-09-14 17:00         ` Jens Axboe
2017-09-14 17:11           ` Bjorn Helgaas
2017-09-14 17:22             ` Jens Axboe
2017-09-14 17:28               ` Srinath Mannam
2017-09-14 17:35                 ` Jens Axboe
2017-09-14 17:44                   ` Srinath Mannam
2017-09-14 17:45                     ` Jens Axboe
2017-09-14 17:50                     ` Johannes Berg
2017-09-14 17:44                   ` Jens Axboe
2017-09-14 20:04                     ` Srinath Mannam
2017-09-14 20:36                       ` Jens Axboe
2017-09-15 19:32                         ` Jens Axboe
2017-09-15 19:36                           ` Luca Coelho
2017-09-15 19:46                             ` Jens Axboe
2017-09-15 19:38                           ` Linus Torvalds
2017-09-15 19:43                             ` Luca Coelho
2017-09-15 19:51                               ` Linus Torvalds
2017-09-15 19:57                                 ` Jens Axboe
2017-09-15 19:48                             ` Jens Axboe
2017-09-15 19:51                               ` Luca Coelho
2017-09-15 19:55                                 ` Jens Axboe
2017-09-16  3:03                                   ` Bjorn Helgaas
2017-09-16 18:53                                     ` Jens Axboe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.