Linux-Wireless Archive on lore.kernel.org
 help / color / Atom feed
From: Jens Axboe <axboe@kernel.dk>
To: Srinath Mannam <srinath.mannam@broadcom.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	Johannes Berg <johannes@sipsolutions.net>,
	Luca Coelho <luca@coelho.fi>,
	"Grumbach, Emmanuel" <emmanuel.grumbach@intel.com>,
	linuxwifi <linuxwifi@intel.com>,
	"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: iwlwifi firmware load broken in current -git
Date: Fri, 15 Sep 2017 13:32:26 -0600
Message-ID: <ee04c95d-167b-0782-1860-253a2271d66c@kernel.dk> (raw)
In-Reply-To: <a5fd0c11-916f-1404-c979-2fd566ed72ec@kernel.dk>

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

  reply index

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-12 15:48 Jens Axboe
2017-09-12 16:11 ` Coelho, Luciano
2017-09-12 16:35   ` Jens Axboe
2017-09-12 16:36   ` Luca Coelho
2017-09-12 16:59     ` Jens Axboe
2017-09-12 19:43     ` Jens Axboe
2017-09-12 19:51       ` Luca Coelho
2017-09-12 20:04       ` Johannes Berg
2017-09-14 17:00         ` Jens Axboe
2017-09-14 17:11           ` Bjorn Helgaas
2017-09-14 17:22             ` Jens Axboe
2017-09-14 17:28               ` Srinath Mannam
2017-09-14 17:35                 ` Jens Axboe
2017-09-14 17:44                   ` Srinath Mannam
2017-09-14 17:45                     ` Jens Axboe
2017-09-14 17:50                     ` Johannes Berg
2017-09-14 17:44                   ` Jens Axboe
2017-09-14 20:04                     ` Srinath Mannam
2017-09-14 20:36                       ` Jens Axboe
2017-09-15 19:32                         ` Jens Axboe [this message]
2017-09-15 19:36                           ` Luca Coelho
2017-09-15 19:46                             ` Jens Axboe
2017-09-15 19:38                           ` Linus Torvalds
2017-09-15 19:43                             ` Luca Coelho
2017-09-15 19:51                               ` Linus Torvalds
2017-09-15 19:57                                 ` Jens Axboe
2017-09-15 19:48                             ` Jens Axboe
2017-09-15 19:51                               ` Luca Coelho
2017-09-15 19:55                                 ` Jens Axboe
2017-09-16  3:03                                   ` Bjorn Helgaas
2017-09-16 18:53                                     ` Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ee04c95d-167b-0782-1860-253a2271d66c@kernel.dk \
    --to=axboe@kernel.dk \
    --cc=bhelgaas@google.com \
    --cc=emmanuel.grumbach@intel.com \
    --cc=johannes@sipsolutions.net \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=linuxwifi@intel.com \
    --cc=luca@coelho.fi \
    --cc=srinath.mannam@broadcom.com \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Wireless Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-wireless/0 linux-wireless/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-wireless linux-wireless/ https://lore.kernel.org/linux-wireless \
		linux-wireless@vger.kernel.org
	public-inbox-index linux-wireless

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-wireless


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git