Re: [PATCH 10/20] qla2xxx: Fix interaction issue between qla2xxx and Target Core Module

From: Quinn Tran <quinn.tran@qlogic.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: Himanshu Madhani <himanshu.madhani@qlogic.com>,
	"target-devel@vger.kernel.org" <target-devel@vger.kernel.org>,
	"nab@linux-iscsi.org" <nab@linux-iscsi.org>,
	Giridhar Malavali <giridhar.malavali@qlogic.com>,
	linux-scsi <linux-scsi@vger.kernel.org>
Subject: Re: [PATCH 10/20] qla2xxx: Fix interaction issue between qla2xxx and Target Core Module
Date: Mon, 14 Dec 2015 21:59:11 +0000	[thread overview]
Message-ID: <D2947AEC.1B91E%quinn.tran@qlogic.com> (raw)
In-Reply-To: <20151214103449.GA8095@infradead.org>

[-- Attachment #1: Type: text/plain, Size: 1716 bytes --]

Christoph,  Thanks for reviewing.  I¹ll withdraw this patch.  Will rework
with new code and submit at a later time.

Regards,
Quinn Tran

On 12/14/15, 2:34 AM, "Christoph Hellwig" <hch@infradead.org> wrote:

>On Wed, Dec 09, 2015 at 10:07:32PM +0000, Quinn Tran wrote:
>> >Err, no.  Looking into the refcount inside a kref is never the
>> >right thing to do.
>> 
>> QT> even for debug purpose??
>
>No.  Please treat struct kref as opaque.
>
>> QT> These bits provide indication as to where the command has traversed
>>in
>> the QLA code.  Each bit is set one time. Due to the async nature of the
>> TMR code, it triggers QLA driver to repeat this specific free path in
>>the
>> double free case.  This BUG_ON allows us trap it early on.
>> 
>> In one of the corner case (below), I need to overloaded it + lock for
>>the
>> cleanup process.
>
>Setting bits fundamentaly is a read/modify/write cycle.  You either
>need to use {set,clear,test}_bit or lock around these manipulations.
>
>> QT> The cmd->aborted flag is used to track the CMD_T_ABORT flag at TCM
>> level.  If the command have been requested to be aborted by TCM or
>>already
>> aborted, we advance it to the ?free" state because our hardware have
>> already started freeing up resources associated to this
>>command/exchange.
>> In this specific case(above), a XFER RDY was aborted by the TMR.
>> Returning the cmd to TCM to generate SCSI Status would generate
>>erroneous
>> HW error due to freed resource.
>
>I really think this nees to be updated on top of Bat's changes as a
>start and re-reviewed.  The amoutn of special casing and second guessing
>here is simply not sustainable in the long run.

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 5204 bytes --]