Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 31 May 2021 11:55:43 +0200
From:   Daniel Wagner <dwagner@suse.de>
To:     Arun Easi <aeasi@marvell.com>
Cc:     linux-scsi@vger.kernel.org, GR-QLogic-Storage-Upstream@marvell.com,
        linux-kernel@vger.kernel.org, Nilesh Javali <njavali@marvell.com>
Subject: Re: [EXT] [RFC 0/2] Serialize timeout handling and done callback.
Message-ID: <20210531095543.gszkwzrmx4nhpfqn@beryllium.lan>
References: <20210507123103.10265-1-dwagner@suse.de>
 <alpine.LRH.2.21.9999.2105310148300.17918@irv1user01.caveonetworks.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.LRH.2.21.9999.2105310148300.17918@irv1user01.caveonetworks.com>
Precedence: bulk

Hi Arun,

On Mon, May 31, 2021 at 02:06:24AM -0700, Arun Easi wrote:
> Thanks Daniel for the report and your effort here. Agree, this needs to be 
> fixed.

Good to hear!

> If you do not mind, can I take this from here? This touches quite a 
> number of paths, and I would like to have this go through a full 
> regression cycle before this is merged.

Sure, that is what I hoped for. It is an invasive change and this needs
to be properly tested with a few different setups. Something I can't really
do. So I would be glad if you could pick up the patches and fix them up.

> That said, I had some general comments:
> 
> 1. I see sp->lock was introduced, but could not locate where it was
> used.

I thought I needed it for serializing the kref operations. The lock
itself is not used in the driver. After re-reading the documentation,
the lock is not necessary as kref_put() is able to serialize the ref
counter inc/dec operation correctly. The lock would only be useful to
serialize the kref_put() with something which runs in the driver
concurrently.

> 2. I did not see a release of lock after a successful kref_put_lock, maybe 
>    that piece was missed out.

I think you got it right. The lock is not necessary.

> 3. The sp->done should be invoked only once, and I do not see if this is
>    taken care of.

qla2x00_sp_release() will only be called when the ref counter gets 0.
This makes sure we only call sp->done() once.

> 4. sp->cmd_sp could be a SCSI IO too, where sp is not allocated 
>    separately, so qla2x00_sp_release shall not be called for it.

Okay, didn't realize this.

Thanks,
Daniel