MIME-Version: 1.0
References: <20190207014143.41529-1-briannorris@chromium.org>
 <20190325202706.GA68720@google.com> <CABvG-CWODF24BMeoCE98OK5fnGNgX0k3z5qTU8A0AD9ncH0jHw@mail.gmail.com>
 <CA+ASDXMRXMSFPXfzbG_2wKzBYAZmp2AVO1qsAJdALLOkPrVRCw@mail.gmail.com>
In-Reply-To: <CA+ASDXMRXMSFPXfzbG_2wKzBYAZmp2AVO1qsAJdALLOkPrVRCw@mail.gmail.com>
From:   Brian Norris <briannorris@chromium.org>
Date:   Tue, 26 Mar 2019 13:35:12 -0700
Message-ID: <CA+ASDXO+qTj4FLxSQXQ7wLHU-OuY0taFkdRfb0fkEEYQwZRY9A@mail.gmail.com>
Subject: Re: [PATCH] ath10k: pci: use mutex for diagnostic window CE polling
To:     =?UTF-8?Q?Micha=C5=82_Kazior?= <kazikcz@gmail.com>
Cc:     Kalle Valo <kvalo@codeaurora.org>,
        Carl Huang <cjhuang@codeaurora.org>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        ath10k@lists.infradead.org, Wen Gong <wgong@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Mon, Mar 25, 2019 at 3:14 PM Brian Norris <briannorris@chromium.org> wro=
te:
> On Mon, Mar 25, 2019 at 2:20 PM Micha=C5=82 Kazior <kazikcz@gmail.com> wr=
ote:
> > For one, you'll need to defer ath10k_pci_fw_crashed_dump to a worker.
> > Maybe into ar->restart_work which the dump function calls now.
>
> Hmm, that's an idea -- although I'm not sure if I'd steal
> 'restart_work', or create a different work item on the same queue. But
> either way, we'd still also have to avoid holding 'data_lock', and at
> that point, I'm not sure if we're losing desirable properties of these
> firmware dumps -- it allows more "stuff" to keep going on while we're
> preparing to dump the device memory state.

So IIUC, we don't today have, for instance, a complete guarantee that
Copy Engines are stopped at this point, so these memory dumps are
useful mostly by the implied fact that a crashed firmware is no longer
doing anything active.

So as long as we ensure the driver is retaining exclusion on its own
resources (e.g., coredump buffers) while performing the dump work,
then we should be OK.

> > To get rid of data_lock from ath10k_pci_fw_crashed_dump() you'll need
> > to at least make fw_crash_counter into an atomic_t. This is just from
> > a quick glance.
>
> Yes, we'd need at least that much. We'd also need some other form of
> locking to ensure exclusion between all users of
> ar->coredump.fw_crash_data and similar. At the moment, that's
> 'data_lock', but I suppose we get a similar exclusion if all the
> dump/restart work is on the same workqueue.

I've cooked up a solution with a new dump_{work,mutex} to protect the
coredump buffers and do the dumping work. I still keep the 'data_lock'
only around the fw_crash_counter.

> I'm still not quite sure if this is 5.1-rc material, or I should just
> revert for 5.1.

I'll post the above soon with a goal of 5.1. If that's not considered
good enough, i'll post a revert too.

Brian