Re: iwlwifi firmware load broken in current -git

Re: iwlwifi firmware load broken in current -git

[Bjorn Helgaas]

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

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

Re: iwlwifi firmware load broken in current -git

[Srinath Mannam]

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
>

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

Re: iwlwifi firmware load broken in current -git

[Srinath Mannam]

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.

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

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

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

Re: iwlwifi firmware load broken in current -git

[Srinath Mannam]

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
>

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

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

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

Re: iwlwifi firmware load broken in current -git

[Linus Torvalds]

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

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

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

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

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

Re: iwlwifi firmware load broken in current -git

[Linus Torvalds]

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

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

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

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

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