Re: [PATCH v3] switchtec: Fix stdev_release crash after suprise device loss.

From: Dmitry Safonov <dima@arista.com>
To: Bjorn Helgaas <helgaas@kernel.org>, Daniel Stodden <dns@arista.com>
Cc: Kurt Schwemmer <kurt.schwemmer@microsemi.com>,
	Logan Gunthorpe <logang@deltatee.com>,
	linux-pci@vger.kernel.org
Subject: Re: [PATCH v3] switchtec: Fix stdev_release crash after suprise device loss.
Date: Wed, 22 Nov 2023 00:25:48 +0000	[thread overview]
Message-ID: <eb27909b-931e-406e-a77d-b77f7305b532@arista.com> (raw)
In-Reply-To: <20231122001922.GA264028@bhelgaas>

HI Bjorn,

On 11/22/23 00:19, Bjorn Helgaas wrote:
> On Tue, Nov 21, 2023 at 03:58:22PM -0800, Daniel Stodden wrote:
>>> On Nov 21, 2023, at 10:40 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> On Tue, Nov 21, 2023 at 10:23:51AM -0800, Daniel Stodden wrote:
>>>>> On Nov 20, 2023, at 1:25 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>> On Mon, Nov 13, 2023 at 01:21:50PM -0800, Daniel Stodden wrote:
>>>>>> A pci device hot removal may occur while stdev->cdev is held open. The
>>>>>> call to stdev_release is then delivered during close or exit, at a
>>>>>> point way past switchtec_pci_remove. Otherwise the last ref would
>>>>>> vanish with the trailing put_device, just before return.
>>>>>>
>>>>>> At that later point in time, the device layer has alreay removed
>>>>>> stdev->mrpc_mmio map. Also, the stdev->pdev reference was not a
>>>>>
>>>>> I guess this should say the "stdev->mmio_mrpc" (not "mrpc_mmio")?
>>>>
>>>> Eww. My fault. Could you still correct that?
>>>
>>> Yep, I speculatively made that change already, so thanks for
>>> confirming it :)
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=switchtec&id=f9724598e29d
>>
>> Thanks. And sorry for what’s next. 
>>
>> Look what I just found in my internal review inbox.
>>
>> Signed-off/Reviewed-by: dima@arista.com
> 
> Happy to add that, no problem, but:
> 
>   - "Signed-off-by" has a specific meaning about either being involved
>     in the creation of the patch or being part of the chain of
>     transmitting the patch, see
>     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.6#n396
>     Since I got the patch directly from you, adding a signed-off-by
>     from dima@ would mean he created part of the patch.
> 
>   - I don't think it makes sense to include both Signed-off-by and
>     Reviewed-by from the same person, since the point of Reviewed-by
>     is to get review by somebody other than the author.
> 
>   - dima@ should include a name as well as the email address (I assume
>     "Dmitry Safonov <dima@arista.com>")
> 
> So if you want to use a Signed-off-by from Dmitry, please post a v4
> including that (ideally starting from the commit above because I
> silently fixed a couple other typos (although I missed the
> put_device() thing)).
> 
> If you prefer a Dmitry's Reviewed-by, no need to post a v4.  The best
> thing would be for Dmitry to respond with the Reviewed-by on the
> mailing list.  Some people do collect reviews internally, but I prefer
> to get them directly from the reviewer.

It's fine to drop my SoB. With the fixup above,

Reviewed-by: Dmitry Safonov <dima@arista.com>

Thanks,
            Dmitry