Linux-Wireless Archive on lore.kernel.org
 help / color / Atom feed
From: Srinath Mannam <srinath.mannam@broadcom.com>
To: Jens Axboe <axboe@kernel.dk>
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>
Subject: Re: iwlwifi firmware load broken in current -git
Date: Fri, 15 Sep 2017 01:34:29 +0530
Message-ID: <CABe79T79WhBh66iQB-5LLCQO0d37evg5tsKUGkj7+z8vNGTHcA@mail.gmail.com> (raw)
In-Reply-To: <93bfbc9b-8829-66fa-f4d6-6d47124e460c@kernel.dk>

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
>

  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 [this message]
2017-09-14 20:36                       ` Jens Axboe
2017-09-15 19:32                         ` Jens Axboe
2017-09-15 19:36                           ` Luca Coelho
2017-09-15 19:46                             ` Jens Axboe
2017-09-15 19:38                           ` Linus Torvalds
2017-09-15 19:43                             ` Luca Coelho
2017-09-15 19:51                               ` Linus Torvalds
2017-09-15 19:57                                 ` Jens Axboe
2017-09-15 19:48                             ` Jens Axboe
2017-09-15 19:51                               ` Luca Coelho
2017-09-15 19:55                                 ` Jens Axboe
2017-09-16  3:03                                   ` Bjorn Helgaas
2017-09-16 18:53                                     ` Jens Axboe

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=CABe79T79WhBh66iQB-5LLCQO0d37evg5tsKUGkj7+z8vNGTHcA@mail.gmail.com \
    --to=srinath.mannam@broadcom.com \
    --cc=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 \
    /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