Re: iwlwifi firmware load broken in current -git

[Jens Axboe]

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  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

Re: iwlwifi firmware load broken in current -git

[Bjorn Helgaas]

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  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

Re: iwlwifi firmware load broken in current -git

[Jens Axboe]

On 09/15/2017 01:51 PM, Linus Torvalds wrote:
> On Fri, Sep 15, 2017 at 12:43 PM, Luca Coelho  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

Re: iwlwifi firmware load broken in current -git

[Jens Axboe]

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  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

Re: iwlwifi firmware load broken in current -git

[Linus Torvalds]

On Fri, Sep 15, 2017 at 12:43 PM, Luca Coelho  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

Re: iwlwifi firmware load broken in current -git

[Luca Coelho]

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  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.

Re: iwlwifi firmware load broken in current -git

[Jens Axboe]

On 09/15/2017 01:38 PM, Linus Torvalds wrote:
> On Fri, Sep 15, 2017 at 12:32 PM, Jens Axboe  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

Re: iwlwifi firmware load broken in current -git

[Jens Axboe]

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  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  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  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(_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(>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

Re: iwlwifi firmware load broken in current -git

[Luca Coelho]

On Fri, 2017-09-15 at 12:38 -0700, Linus Torvalds wrote:
> On Fri, Sep 15, 2017 at 12:32 PM, Jens Axboe  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.

Re: iwlwifi firmware load broken in current -git

[Linus Torvalds]

On Fri, Sep 15, 2017 at 12:32 PM, Jens Axboe  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

Re: iwlwifi firmware load broken in current -git

[Luca Coelho]

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  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  
> > > > > > 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  
> > > > > > > > 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(_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(>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 

Re: iwlwifi firmware load broken in current -git

[Jens Axboe]

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  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  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  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(_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(>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

Re: iwlwifi firmware load broken in current -git

[Jens Axboe]

On 09/14/2017 02:04 PM, Srinath Mannam wrote:
> Hi Jens Axboe,
> 
> 
> On Thu, Sep 14, 2017 at 11:14 PM, Jens Axboe  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  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  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(_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(>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

Re: iwlwifi firmware load broken in current -git

[Srinath Mannam]

Hi Jens Axboe,


On Thu, Sep 14, 2017 at 11:14 PM, Jens Axboe  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  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  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(_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(>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
>

Re: iwlwifi firmware load broken in current -git

[Johannes Berg]

On Thu, 2017-09-14 at 23:14 +0530, Srinath Mannam wrote:
> 
> atomic_inc_return(>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

Re: iwlwifi firmware load broken in current -git

[Jens Axboe]

On 09/14/2017 11:44 AM, Srinath Mannam wrote:
> Hi Jens Axboe,
> 
> On Thu, Sep 14, 2017 at 11:05 PM, 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  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  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(_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(>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
-[: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

Re: iwlwifi firmware load broken in current -git

[Jens Axboe]

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  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  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(_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(>enable_cnt) > 1)
return 0;   /* already enabled */

-- 
Jens Axboe

Re: iwlwifi firmware load broken in current -git

[Srinath Mannam]

Hi Jens Axboe,

On Thu, Sep 14, 2017 at 11:05 PM, 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  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  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(_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(>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.

Re: iwlwifi firmware load broken in current -git

[Jens Axboe]

On 09/14/2017 11:28 AM, Srinath Mannam wrote:
> Hi Bjorn,
> 
> On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe  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  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(_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

Re: iwlwifi firmware load broken in current -git

[Srinath Mannam]

Hi Bjorn,

On Thu, Sep 14, 2017 at 10:52 PM, Jens Axboe  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  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(_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
>

Re: iwlwifi firmware load broken in current -git

[Jens Axboe]

On 09/14/2017 11:11 AM, Bjorn Helgaas wrote:
> [+cc linux-pci]
> 
> On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe  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

Re: iwlwifi firmware load broken in current -git

[Bjorn Helgaas]

[+cc linux-pci]

On Thu, Sep 14, 2017 at 12:00 PM, Jens Axboe  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

Re: iwlwifi firmware load broken in current -git

[Jens Axboe]

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

Re: iwlwifi firmware load broken in current -git

[Johannes Berg]

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

Re: iwlwifi firmware load broken in current -git

[Luca Coelho]

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 +, 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 :04:00.0: Direct firmware load for 
> > > > iwlwifi-8000C-34.ucode failed with error -2
> > > > [4.363733] iwlwifi :04:00.0: Direct firmware load for 
> > > > iwlwifi-8000C-33.ucode failed with error -2
> > > > [4.363744] iwlwifi :04:00.0: Direct firmware load for 
> > > > iwlwifi-8000C-32.ucode failed with error -2
> > > > [4.368077] iwlwifi :04:00.0: loaded firmware version 
> > > > 31.532993.0 op_mode iwlmvm
> > > > [4.461753] iwlwifi :04:00.0: Detected Intel(R) Dual Band 
> > > > Wireless AC 8260, REV=0x208
> > > > [ ... ]
> > > > [9.510570] iwlwifi :04:00.0: Failed to load firmware chunk! 
> > > > 
> > > > [9.513903] iwlwifi :04:00.0: Could not load the [0] uCode 
> > > > section   
> > > > [9.516201] iwlwifi :04:00.0: Failed to start INIT ucode: -110   
> > > > 
> > > > [9.530842] iwlwifi :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 :04:00.0: Direct firmware load for 
> > > > iwlwifi-8000C-34.ucode failed with error -2
> > > > [   60.991941] iwlwifi :04:00.0: Direct firmware load for 
> > > > iwlwifi-8000C-33.ucode failed with error -2
> > > > [   60.991946] iwlwifi :04:00.0: Direct firmware load for 
> > > > iwlwifi-8000C-32.ucode failed with error -2
> > > > [   60.991957] iwlwifi :04:00.0: Direct firmware load for 
> > > > iwlwifi-8000C-31.ucode failed with error -2
> > > > [   60.991964] iwlwifi :04:00.0: Direct firmware load for 
> > > > iwlwifi-8000C-30.ucode failed with error -2
> > > > [   60.991969] iwlwifi :04:00.0: Direct firmware load for 
> > > > iwlwifi-8000C-29.ucode failed with error -2
> > > > [   60.991975] iwlwifi :04:00.0: Direct firmware load for 
> > > > iwlwifi-8000C-28.ucode failed with error -2
> > > > [   61.029852] iwlwifi :04:00.0: loaded firmware version 
> > > > 27.541033.0 op_mode iwlmvm
> > > > [   61.043660] iwlwifi :04:00.0: Detected Intel(R) Dual Band 
> > > > Wireless AC 8260, REV=0x208
> > > > [   66.070135] iwlwifi :04:00.0: Failed to load firmware chunk! 
> > > > 
> > > > [   66.070149] iwlwifi :04:00.0: Could not load the [0] uCode 
> > > > section   
> > > > [   66.070165] iwlwifi :04:00.0: Failed to start INIT ucode: -110   
> > > > 
> > > > [   66.083462] iwlwifi :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 :02:00.0: Direct firmware load for 
> > iwlwifi-8000C-34.ucode failed with error -2
> > [8.995136] iwlwifi :02:00.0: Direct firmware load for 
> > iwlwifi-8000C-33.ucode failed with error -2
> > [9.026455] iwlwifi :02:00.0: Direct firmware load for 
> > iwlwifi-8000C-32.ucode failed with error -2
> > [9.348325] iwlwifi :02:00.0: loaded firmware version 31.532993.0 
> > op_mode iwlmvm
> > [9.369307] iwlwifi :02:00.0: Detected Intel(R) Dual Band Wireless 
> > AC 8260, REV=0x208
> > [9.435915] iwlwifi :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 
> Date:   Fri Aug 18 21:50:48 2017 -0500
> 

Re: iwlwifi firmware load broken in current -git

[Jens Axboe]

On 09/12/2017 10:36 AM, Luca Coelho wrote:
> On Tue, 2017-09-12 at 16:11 +, 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 :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-34.ucode failed with error -2
>>> [4.363733] iwlwifi :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-33.ucode failed with error -2
>>> [4.363744] iwlwifi :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-32.ucode failed with error -2
>>> [4.368077] iwlwifi :04:00.0: loaded firmware version 31.532993.0 
>>> op_mode iwlmvm
>>> [4.461753] iwlwifi :04:00.0: Detected Intel(R) Dual Band Wireless 
>>> AC 8260, REV=0x208
>>> [ ... ]
>>> [9.510570] iwlwifi :04:00.0: Failed to load firmware chunk! 
>>> 
>>> [9.513903] iwlwifi :04:00.0: Could not load the [0] uCode section   
>>> 
>>> [9.516201] iwlwifi :04:00.0: Failed to start INIT ucode: -110   
>>> 
>>> [9.530842] iwlwifi :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 :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-34.ucode failed with error -2
>>> [   60.991941] iwlwifi :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-33.ucode failed with error -2
>>> [   60.991946] iwlwifi :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-32.ucode failed with error -2
>>> [   60.991957] iwlwifi :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-31.ucode failed with error -2
>>> [   60.991964] iwlwifi :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-30.ucode failed with error -2
>>> [   60.991969] iwlwifi :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-29.ucode failed with error -2
>>> [   60.991975] iwlwifi :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-28.ucode failed with error -2
>>> [   61.029852] iwlwifi :04:00.0: loaded firmware version 27.541033.0 
>>> op_mode iwlmvm
>>> [   61.043660] iwlwifi :04:00.0: Detected Intel(R) Dual Band Wireless 
>>> AC 8260, REV=0x208
>>> [   66.070135] iwlwifi :04:00.0: Failed to load firmware chunk! 
>>> 
>>> [   66.070149] iwlwifi :04:00.0: Could not load the [0] uCode section   
>>> 
>>> [   66.070165] iwlwifi :04:00.0: Failed to start INIT ucode: -110   
>>> 
>>> [   66.083462] iwlwifi :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 :02:00.0: Direct firmware load for 
> iwlwifi-8000C-34.ucode failed with error -2
> [8.995136] iwlwifi :02:00.0: Direct firmware load for 
> iwlwifi-8000C-33.ucode failed with error -2
> [9.026455] iwlwifi :02:00.0: Direct firmware load for 
> iwlwifi-8000C-32.ucode failed with error -2
> [9.348325] iwlwifi :02:00.0: loaded firmware version 31.532993.0 
> op_mode iwlmvm
> [9.369307] iwlwifi :02:00.0: Detected Intel(R) Dual Band Wireless AC 
> 8260, REV=0x208
> [9.435915] iwlwifi :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 
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

Re: iwlwifi firmware load broken in current -git

[Jens Axboe]

On 09/12/2017 10:36 AM, Luca Coelho wrote:
> On Tue, 2017-09-12 at 16:11 +, 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 :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-34.ucode failed with error -2
>>> [4.363733] iwlwifi :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-33.ucode failed with error -2
>>> [4.363744] iwlwifi :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-32.ucode failed with error -2
>>> [4.368077] iwlwifi :04:00.0: loaded firmware version 31.532993.0 
>>> op_mode iwlmvm
>>> [4.461753] iwlwifi :04:00.0: Detected Intel(R) Dual Band Wireless 
>>> AC 8260, REV=0x208
>>> [ ... ]
>>> [9.510570] iwlwifi :04:00.0: Failed to load firmware chunk! 
>>> 
>>> [9.513903] iwlwifi :04:00.0: Could not load the [0] uCode section   
>>> 
>>> [9.516201] iwlwifi :04:00.0: Failed to start INIT ucode: -110   
>>> 
>>> [9.530842] iwlwifi :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 :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-34.ucode failed with error -2
>>> [   60.991941] iwlwifi :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-33.ucode failed with error -2
>>> [   60.991946] iwlwifi :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-32.ucode failed with error -2
>>> [   60.991957] iwlwifi :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-31.ucode failed with error -2
>>> [   60.991964] iwlwifi :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-30.ucode failed with error -2
>>> [   60.991969] iwlwifi :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-29.ucode failed with error -2
>>> [   60.991975] iwlwifi :04:00.0: Direct firmware load for 
>>> iwlwifi-8000C-28.ucode failed with error -2
>>> [   61.029852] iwlwifi :04:00.0: loaded firmware version 27.541033.0 
>>> op_mode iwlmvm
>>> [   61.043660] iwlwifi :04:00.0: Detected Intel(R) Dual Band Wireless 
>>> AC 8260, REV=0x208
>>> [   66.070135] iwlwifi :04:00.0: Failed to load firmware chunk! 
>>> 
>>> [   66.070149] iwlwifi :04:00.0: Could not load the [0] uCode section   
>>> 
>>> [   66.070165] iwlwifi :04:00.0: Failed to start INIT ucode: -110   
>>> 
>>> [   66.083462] iwlwifi :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 :02:00.0: Direct firmware load for 
> iwlwifi-8000C-34.ucode failed with error -2
> [8.995136] iwlwifi :02:00.0: Direct firmware load for 
> iwlwifi-8000C-33.ucode failed with error -2
> [9.026455] iwlwifi :02:00.0: Direct firmware load for 
> iwlwifi-8000C-32.ucode failed with error -2
> [9.348325] iwlwifi :02:00.0: loaded firmware version 31.532993.0 
> op_mode iwlmvm
> [9.369307] iwlwifi :02:00.0: Detected Intel(R) Dual Band Wireless AC 
> 8260, REV=0x208
> [9.435915] iwlwifi :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

Re: iwlwifi firmware load broken in current -git

[Luca Coelho]

On Tue, 2017-09-12 at 16:11 +, 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 :04:00.0: Direct firmware load for 
> > iwlwifi-8000C-34.ucode failed with error -2
> > [4.363733] iwlwifi :04:00.0: Direct firmware load for 
> > iwlwifi-8000C-33.ucode failed with error -2
> > [4.363744] iwlwifi :04:00.0: Direct firmware load for 
> > iwlwifi-8000C-32.ucode failed with error -2
> > [4.368077] iwlwifi :04:00.0: loaded firmware version 31.532993.0 
> > op_mode iwlmvm
> > [4.461753] iwlwifi :04:00.0: Detected Intel(R) Dual Band Wireless 
> > AC 8260, REV=0x208
> > [ ... ]
> > [9.510570] iwlwifi :04:00.0: Failed to load firmware chunk! 
> > 
> > [9.513903] iwlwifi :04:00.0: Could not load the [0] uCode section   
> > 
> > [9.516201] iwlwifi :04:00.0: Failed to start INIT ucode: -110   
> > 
> > [9.530842] iwlwifi :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 :04:00.0: Direct firmware load for 
> > iwlwifi-8000C-34.ucode failed with error -2
> > [   60.991941] iwlwifi :04:00.0: Direct firmware load for 
> > iwlwifi-8000C-33.ucode failed with error -2
> > [   60.991946] iwlwifi :04:00.0: Direct firmware load for 
> > iwlwifi-8000C-32.ucode failed with error -2
> > [   60.991957] iwlwifi :04:00.0: Direct firmware load for 
> > iwlwifi-8000C-31.ucode failed with error -2
> > [   60.991964] iwlwifi :04:00.0: Direct firmware load for 
> > iwlwifi-8000C-30.ucode failed with error -2
> > [   60.991969] iwlwifi :04:00.0: Direct firmware load for 
> > iwlwifi-8000C-29.ucode failed with error -2
> > [   60.991975] iwlwifi :04:00.0: Direct firmware load for 
> > iwlwifi-8000C-28.ucode failed with error -2
> > [   61.029852] iwlwifi :04:00.0: loaded firmware version 27.541033.0 
> > op_mode iwlmvm
> > [   61.043660] iwlwifi :04:00.0: Detected Intel(R) Dual Band Wireless 
> > AC 8260, REV=0x208
> > [   66.070135] iwlwifi :04:00.0: Failed to load firmware chunk! 
> > 
> > [   66.070149] iwlwifi :04:00.0: Could not load the [0] uCode section   
> > 
> > [   66.070165] iwlwifi :04:00.0: Failed to start INIT ucode: -110   
> > 
> > [   66.083462] iwlwifi :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 :02:00.0: Direct firmware load for 
iwlwifi-8000C-34.ucode failed with error -2
[8.995136] iwlwifi :02:00.0: Direct firmware load for 
iwlwifi-8000C-33.ucode failed with error -2
[9.026455] iwlwifi :02:00.0: Direct firmware load for 
iwlwifi-8000C-32.ucode failed with error -2
[9.348325] iwlwifi :02:00.0: loaded firmware version 31.532993.0 
op_mode iwlmvm
[9.369307] iwlwifi :02:00.0: Detected Intel(R) Dual Band Wireless AC 
8260, REV=0x208
[9.435915] iwlwifi :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.

Re: iwlwifi firmware load broken in current -git

[Jens Axboe]

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 :04:00.0: Direct firmware load for 
>> iwlwifi-8000C-34.ucode failed with error -2
>> [4.363733] iwlwifi :04:00.0: Direct firmware load for 
>> iwlwifi-8000C-33.ucode failed with error -2
>> [4.363744] iwlwifi :04:00.0: Direct firmware load for 
>> iwlwifi-8000C-32.ucode failed with error -2
>> [4.368077] iwlwifi :04:00.0: loaded firmware version 31.532993.0 
>> op_mode iwlmvm
>> [4.461753] iwlwifi :04:00.0: Detected Intel(R) Dual Band Wireless AC 
>> 8260, REV=0x208
>> [ ... ]
>> [9.510570] iwlwifi :04:00.0: Failed to load firmware chunk!  
>>
>> [9.513903] iwlwifi :04:00.0: Could not load the [0] uCode section
>>
>> [9.516201] iwlwifi :04:00.0: Failed to start INIT ucode: -110
>>
>> [9.530842] iwlwifi :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 :04:00.0: Direct firmware load for 
>> iwlwifi-8000C-34.ucode failed with error -2
>> [   60.991941] iwlwifi :04:00.0: Direct firmware load for 
>> iwlwifi-8000C-33.ucode failed with error -2
>> [   60.991946] iwlwifi :04:00.0: Direct firmware load for 
>> iwlwifi-8000C-32.ucode failed with error -2
>> [   60.991957] iwlwifi :04:00.0: Direct firmware load for 
>> iwlwifi-8000C-31.ucode failed with error -2
>> [   60.991964] iwlwifi :04:00.0: Direct firmware load for 
>> iwlwifi-8000C-30.ucode failed with error -2
>> [   60.991969] iwlwifi :04:00.0: Direct firmware load for 
>> iwlwifi-8000C-29.ucode failed with error -2
>> [   60.991975] iwlwifi :04:00.0: Direct firmware load for 
>> iwlwifi-8000C-28.ucode failed with error -2
>> [   61.029852] iwlwifi :04:00.0: loaded firmware version 27.541033.0 
>> op_mode iwlmvm
>> [   61.043660] iwlwifi :04:00.0: Detected Intel(R) Dual Band Wireless AC 
>> 8260, REV=0x208
>> [   66.070135] iwlwifi :04:00.0: Failed to load firmware chunk!  
>>
>> [   66.070149] iwlwifi :04:00.0: Could not load the [0] uCode section
>>
>> [   66.070165] iwlwifi :04:00.0: Failed to start INIT ucode: -110
>>
>> [   66.083462] iwlwifi :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

Re: iwlwifi firmware load broken in current -git

[Coelho, Luciano]

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 :04:00.0: Direct firmware load for 
> iwlwifi-8000C-34.ucode failed with error -2
> [4.363733] iwlwifi :04:00.0: Direct firmware load for 
> iwlwifi-8000C-33.ucode failed with error -2
> [4.363744] iwlwifi :04:00.0: Direct firmware load for 
> iwlwifi-8000C-32.ucode failed with error -2
> [4.368077] iwlwifi :04:00.0: loaded firmware version 31.532993.0 
> op_mode iwlmvm
> [4.461753] iwlwifi :04:00.0: Detected Intel(R) Dual Band Wireless AC 
> 8260, REV=0x208
> [ ... ]
> [9.510570] iwlwifi :04:00.0: Failed to load firmware chunk!   
>   
> [9.513903] iwlwifi :04:00.0: Could not load the [0] uCode section 
>   
> [9.516201] iwlwifi :04:00.0: Failed to start INIT ucode: -110 
>   
> [9.530842] iwlwifi :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 :04:00.0: Direct firmware load for 
> iwlwifi-8000C-34.ucode failed with error -2
> [   60.991941] iwlwifi :04:00.0: Direct firmware load for 
> iwlwifi-8000C-33.ucode failed with error -2
> [   60.991946] iwlwifi :04:00.0: Direct firmware load for 
> iwlwifi-8000C-32.ucode failed with error -2
> [   60.991957] iwlwifi :04:00.0: Direct firmware load for 
> iwlwifi-8000C-31.ucode failed with error -2
> [   60.991964] iwlwifi :04:00.0: Direct firmware load for 
> iwlwifi-8000C-30.ucode failed with error -2
> [   60.991969] iwlwifi :04:00.0: Direct firmware load for 
> iwlwifi-8000C-29.ucode failed with error -2
> [   60.991975] iwlwifi :04:00.0: Direct firmware load for 
> iwlwifi-8000C-28.ucode failed with error -2
> [   61.029852] iwlwifi :04:00.0: loaded firmware version 27.541033.0 
> op_mode iwlmvm
> [   61.043660] iwlwifi :04:00.0: Detected Intel(R) Dual Band Wireless AC 
> 8260, REV=0x208
> [   66.070135] iwlwifi :04:00.0: Failed to load firmware chunk!   
>   
> [   66.070149] iwlwifi :04:00.0: Could not load the [0] uCode section 
>   
> [   66.070165] iwlwifi :04:00.0: Failed to start INIT ucode: -110 
>   
> [   66.083462] iwlwifi :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.

--
Cheers,
Luca.