[PATCH 0/3] Remove scsi_cmnd.tag

[PATCH 0/3] Remove scsi_cmnd.tag

[John Garry]

There is no need for scsi_cmnd.tag, so remove it.

Based on next-20210811

John Garry (3):
  scsi: wd719: Stop using scsi_cmnd.tag
  scsi: fnic: Stop setting scsi_cmnd.tag
  scsi: Remove scsi_cmnd.tag

 drivers/scsi/fnic/fnic_scsi.c | 2 +-
 drivers/scsi/scsi_lib.c       | 1 -
 drivers/scsi/wd719x.c         | 8 +++++---
 include/scsi/scsi_cmnd.h      | 1 -
 4 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.26.2

[PATCH 1/3] scsi: wd719: Stop using scsi_cmnd.tag

[John Garry]

Use scsi_cmd_to_rq(cmd)->tag instead.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/wd719x.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/wd719x.c b/drivers/scsi/wd719x.c
index edc8a139a60d..622aec075aba 100644
--- a/drivers/scsi/wd719x.c
+++ b/drivers/scsi/wd719x.c
@@ -466,14 +466,16 @@ static int wd719x_abort(struct scsi_cmnd *cmd)
 	unsigned long flags;
 	struct wd719x_scb *scb = scsi_cmd_priv(cmd);
 	struct wd719x *wd = shost_priv(cmd->device->host);
+	struct device *dev = &wd->pdev->dev;
 
-	dev_info(&wd->pdev->dev, "abort command, tag: %x\n", cmd->tag);
+	dev_info(dev, "abort command, tag: %x\n", scsi_cmd_to_rq(cmd)->tag);
 
-	action = /*cmd->tag ? WD719X_CMD_ABORT_TAG : */WD719X_CMD_ABORT;
+	action = /*tag ? WD719X_CMD_ABORT_TAG : */WD719X_CMD_ABORT;
 
 	spin_lock_irqsave(wd->sh->host_lock, flags);
 	result = wd719x_direct_cmd(wd, action, cmd->device->id,
-				   cmd->device->lun, cmd->tag, scb->phys, 0);
+				   cmd->device->lun, scsi_cmd_to_rq(cmd)->tag,
+				   scb->phys, 0);
 	wd719x_finish_cmd(scb, DID_ABORT);
 	spin_unlock_irqrestore(wd->sh->host_lock, flags);
 	if (result)
-- 
2.26.2

Re: [PATCH 1/3] scsi: wd719: Stop using scsi_cmnd.tag

[Hannes Reinecke]

On 8/13/21 3:49 PM, John Garry wrote:
> Use scsi_cmd_to_rq(cmd)->tag instead.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/scsi/wd719x.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 1/3] scsi: wd719: Stop using scsi_cmnd.tag

[Bart Van Assche]

On 8/13/21 6:49 AM, John Garry wrote:
> -	action = /*cmd->tag ? WD719X_CMD_ABORT_TAG : */WD719X_CMD_ABORT;
> +	action = /*tag ? WD719X_CMD_ABORT_TAG : */WD719X_CMD_ABORT;

If this patch series would be reposted, please remove the commented-out
code instead of modifying it. Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH 2/3] scsi: fnic: Stop setting scsi_cmnd.tag

[John Garry]

It is never read. Setting it and the request tag seems dodgy
anyway.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/fnic/fnic_scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index 0f9cedf78872..f8afbfb468dc 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -2213,7 +2213,7 @@ fnic_scsi_host_start_tag(struct fnic *fnic, struct scsi_cmnd *sc)
 	if (IS_ERR(dummy))
 		return SCSI_NO_TAG;
 
-	sc->tag = rq->tag = dummy->tag;
+	rq->tag = dummy->tag;
 	sc->host_scribble = (unsigned char *)dummy;
 
 	return dummy->tag;
-- 
2.26.2

Re: [PATCH 2/3] scsi: fnic: Stop setting scsi_cmnd.tag

[Hannes Reinecke]

On 8/13/21 3:49 PM, John Garry wrote:
> It is never read. Setting it and the request tag seems dodgy
> anyway.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/scsi/fnic/fnic_scsi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 2/3] scsi: fnic: Stop setting scsi_cmnd.tag

[Bart Van Assche]

On 8/13/21 6:49 AM, John Garry wrote:
> It is never read. Setting it and the request tag seems dodgy
> anyway.

This is done because there is code in the SCSI error handler that may
allocate a SCSI command without allocating a tag. See also
scsi_ioctl_reset().

> ---
>  drivers/scsi/fnic/fnic_scsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
> index 0f9cedf78872..f8afbfb468dc 100644
> --- a/drivers/scsi/fnic/fnic_scsi.c
> +++ b/drivers/scsi/fnic/fnic_scsi.c
> @@ -2213,7 +2213,7 @@ fnic_scsi_host_start_tag(struct fnic *fnic, struct scsi_cmnd *sc)
>  	if (IS_ERR(dummy))
>  		return SCSI_NO_TAG;
>  
> -	sc->tag = rq->tag = dummy->tag;
> +	rq->tag = dummy->tag;
>  	sc->host_scribble = (unsigned char *)dummy;

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 2/3] scsi: fnic: Stop setting scsi_cmnd.tag

[Christoph Hellwig]

On Fri, Aug 13, 2021 at 08:17:45PM -0700, Bart Van Assche wrote:
> On 8/13/21 6:49 AM, John Garry wrote:
> > It is never read. Setting it and the request tag seems dodgy
> > anyway.
> 
> This is done because there is code in the SCSI error handler that may
> allocate a SCSI command without allocating a tag. See also
> scsi_ioctl_reset().

Yes.  Hannes had a great series to stop passing the pointless scsi_cmnd
to the reset methods.  Hannes, any chance you coul look into
resurrecting that?

Re: [PATCH 2/3] scsi: fnic: Stop setting scsi_cmnd.tag

[Hannes Reinecke]

On 8/14/21 9:39 AM, Christoph Hellwig wrote:
> On Fri, Aug 13, 2021 at 08:17:45PM -0700, Bart Van Assche wrote:
>> On 8/13/21 6:49 AM, John Garry wrote:
>>> It is never read. Setting it and the request tag seems dodgy
>>> anyway.
>>
>> This is done because there is code in the SCSI error handler that may
>> allocate a SCSI command without allocating a tag. See also
>> scsi_ioctl_reset().
> 
> Yes.  Hannes had a great series to stop passing the pointless scsi_cmnd
> to the reset methods.  Hannes, any chance you coul look into
> resurrecting that?
> 
Sure.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 2/3] scsi: fnic: Stop setting scsi_cmnd.tag

[John Garry]

On 14/08/2021 13:35, Hannes Reinecke wrote:
> On 8/14/21 9:39 AM, Christoph Hellwig wrote:
>> On Fri, Aug 13, 2021 at 08:17:45PM -0700, Bart Van Assche wrote:
>>> On 8/13/21 6:49 AM, John Garry wrote:
>>>> It is never read. Setting it and the request tag seems dodgy
>>>> anyway.
>>>
>>> This is done because there is code in the SCSI error handler that may
>>> allocate a SCSI command without allocating a tag. See also
>>> scsi_ioctl_reset().

Right, so we just get a loan of the tag of a real request. fnic driver 
comment:

"Really should fix the midlayer to pass in a proper request for ioctls..."

>>
>> Yes.  Hannes had a great series to stop passing the pointless scsi_cmnd
>> to the reset methods.  Hannes, any chance you coul look into
>> resurrecting that?
>>
> Sure.

The latest iteration of that series - at v7 - still passed that fake 
SCSI command to the reset method, and the reset method allocated the 
internal command.

So will we change change scsi_ioctl_reset() to allocate an internal 
command, rather than the LLDD?

Thanks,
John

Re: [PATCH 2/3] scsi: fnic: Stop setting scsi_cmnd.tag

[Hannes Reinecke]

On 8/16/21 12:00 PM, John Garry wrote:
> On 14/08/2021 13:35, Hannes Reinecke wrote:
>> On 8/14/21 9:39 AM, Christoph Hellwig wrote:
>>> On Fri, Aug 13, 2021 at 08:17:45PM -0700, Bart Van Assche wrote:
>>>> On 8/13/21 6:49 AM, John Garry wrote:
>>>>> It is never read. Setting it and the request tag seems dodgy
>>>>> anyway.
>>>>
>>>> This is done because there is code in the SCSI error handler that may
>>>> allocate a SCSI command without allocating a tag. See also
>>>> scsi_ioctl_reset().
> 
> Right, so we just get a loan of the tag of a real request. fnic driver
> comment:
> 
> "Really should fix the midlayer to pass in a proper request for ioctls..."
> 
>>>
>>> Yes.  Hannes had a great series to stop passing the pointless scsi_cmnd
>>> to the reset methods.  Hannes, any chance you coul look into
>>> resurrecting that?
>>>
>> Sure.
> 
> The latest iteration of that series - at v7 - still passed that fake
> SCSI command to the reset method, and the reset method allocated the
> internal command.
> 
> So will we change change scsi_ioctl_reset() to allocate an internal
> command, rather than the LLDD?
> 
Nah, Christoph was talking about my patch series to revamp the SCSI
error handler.
With that one we'll be passing the respective objects to the SCSI EH
functions (ie struct scsi_device for eh_device_reset()), doing away with
the need to allocate an internal command for ioctl reset.
Currently revamping the patchset, should be ready later this week.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

[PATCH 3/3] scsi: Remove scsi_cmnd.tag

[John Garry]

It is never read, so get rid of it.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/scsi_lib.c  | 1 -
 include/scsi/scsi_cmnd.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9ba1aa7530a9..572673873ddf 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1540,7 +1540,6 @@ static blk_status_t scsi_prepare_cmd(struct request *req)
 
 	scsi_init_command(sdev, cmd);
 
-	cmd->tag = req->tag;
 	cmd->prot_op = SCSI_PROT_NORMAL;
 	if (blk_rq_bytes(req))
 		cmd->sc_data_direction = rq_dma_dir(req);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 6c5a1c1c6b1e..eaf04c9a1dfc 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -139,7 +139,6 @@ struct scsi_cmnd {
 	int flags;		/* Command flags */
 	unsigned long state;	/* Command completion state */
 
-	unsigned char tag;	/* SCSI-II queued command tag */
 	unsigned int extra_len;	/* length of alignment and padding */
 };
 
-- 
2.26.2

Re: [PATCH 3/3] scsi: Remove scsi_cmnd.tag

[Hannes Reinecke]

On 8/13/21 3:49 PM, John Garry wrote:
> It is never read, so get rid of it.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/scsi/scsi_lib.c  | 1 -
>   include/scsi/scsi_cmnd.h | 1 -
>   2 files changed, 2 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 3/3] scsi: Remove scsi_cmnd.tag

[Bart Van Assche]

On 8/13/21 6:49 AM, John Garry wrote:
> It is never read, so get rid of it.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 0/3] Remove scsi_cmnd.tag

[Christoph Hellwig]

The whole series looks good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 0/3] Remove scsi_cmnd.tag

[Martin K. Petersen]

John,

> There is no need for scsi_cmnd.tag, so remove it.

Applied to 5.15/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/3] Remove scsi_cmnd.tag

[John Garry]

On 16/08/2021 18:34, Martin K. Petersen wrote:
> 
> John,
> 
>> There is no need for scsi_cmnd.tag, so remove it.
> 
> Applied to 5.15/scsi-staging, thanks!
> 

Hi Martin,

As you may have seen, some arm32 build has also broken. My build 
coverage was not good enough, and I don't see a point in me playing 
whack-a-mole with the kernel test robot. So how about revert the final 
patch or even all of them, and I can try get better build coverage and 
then re-post? Or maybe you or Bart have a better idea?

Thanks!

Re: [PATCH 0/3] Remove scsi_cmnd.tag

[Martin K. Petersen]

John,

> As you may have seen, some arm32 build has also broken. My build
> coverage was not good enough, and I don't see a point in me playing
> whack-a-mole with the kernel test robot. So how about revert the final
> patch or even all of them, and I can try get better build coverage and
> then re-post? Or maybe you or Bart have a better idea?

My due diligence involved:

$ git grep -Ei -- "cmn?d->tag" drivers/scsi

But in retrospect that should have been:

$ git grep -Ei -- "(cmn?d|scpnt)->tag" drivers/scsi

I cringe every time I see "SCpnt" so maybe that's why I didn't think of
it.

Anyway. Please fix the drivers/scsi/arm bits up. There is still time.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/3] Remove scsi_cmnd.tag

[Bart Van Assche]

On 8/18/21 11:08 AM, John Garry wrote:
> Or maybe you or Bart have a better idea?

This is how I test compilation of SCSI drivers on a SUSE system (only
the cross-compilation prefix is distro specific):

    # Acorn RiscPC
    make ARCH=arm xconfig
    # Select the RiscPC architecture (ARCH_RPC)
    make -j9 ARCH=arm CROSS_COMPILE=arm-suse-linux-gnueabi- </dev/null

    # Atari, Amiga
    make ARCH=m68k xconfig<br>
    # Select Amiga + Atari + 68060 + Q40 + SCSI + Zorro +
    # SCSI_FDOMAIN_ISA
    make -j9 ARCH=m68k CROSS_COMPILE=m68k-suse-linux- </dev/null

    # MIPS
    make ARCH=powerpc xconfig<br>
    # Select the SGI IP28 machine type and also the WD93C93 SCSI
    # driver
    make -j9 ARCH=mips CROSS_COMPILE=mips-suse-linux- </dev/null

    # PowerPC
    make ARCH=powerpc xconfig<br>
    # Select the ibmvfc and ibmvscsi drivers<br>
    make -j9 ARCH=powerpc CROSS_COMPILE=powerpc64-suse-linux- \
      </dev/null

    # S/390
    make ARCH=s390 xconfig
    # Select the zfcp driver
    make -j9 ARCH=s390 CROSS_COMPILE=s390x-suse-linux- </dev/null

Bart.

Re: [PATCH 0/3] Remove scsi_cmnd.tag

[Hannes Reinecke]

Hey Bart,

Thanks for this!
Really helpful.

Just a tiny wee snag:

On 8/19/21 4:41 AM, Bart Van Assche wrote:
> On 8/18/21 11:08 AM, John Garry wrote:
>> Or maybe you or Bart have a better idea?
> 
> This is how I test compilation of SCSI drivers on a SUSE system (only
> the cross-compilation prefix is distro specific):
> 
>      # Acorn RiscPC
>      make ARCH=arm xconfig
>      # Select the RiscPC architecture (ARCH_RPC)
>      make -j9 ARCH=arm CROSS_COMPILE=arm-suse-linux-gnueabi- </dev/null
> 

Acorn RiscPC is ARMv3, which sadly isn't supported anymore with gcc9.
So for compilation I had to modify Kconfig to select ARMv4:

diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
index 8355c3895894..22ec9e275335 100644
--- a/arch/arm/mm/Kconfig
+++ b/arch/arm/mm/Kconfig
@@ -278,7 +278,7 @@ config CPU_ARM1026
  # SA110
  config CPU_SA110
         bool
-       select CPU_32v3 if ARCH_RPC
+       select CPU_32v4 if ARCH_RPC
         select CPU_32v4 if !ARCH_RPC
         select CPU_ABRT_EV4
         select CPU_CACHE_V4WB

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 0/3] Remove scsi_cmnd.tag

[John Garry]

On 19/08/2021 08:15, Hannes Reinecke wrote:
> Hey Bart,
> 
> Thanks for this!
> Really helpful.
> 
> Just a tiny wee snag:
> 
> On 8/19/21 4:41 AM, Bart Van Assche wrote:
>> On 8/18/21 11:08 AM, John Garry wrote:
>>> Or maybe you or Bart have a better idea?
>>
>> This is how I test compilation of SCSI drivers on a SUSE system (only
>> the cross-compilation prefix is distro specific):
>>
>>      # Acorn RiscPC
>>      make ARCH=arm xconfig
>>      # Select the RiscPC architecture (ARCH_RPC)
>>      make -j9 ARCH=arm CROSS_COMPILE=arm-suse-linux-gnueabi- </dev/null
>>
> 
> Acorn RiscPC is ARMv3, which sadly isn't supported anymore with gcc9.
> So for compilation I had to modify Kconfig to select ARMv4:
> 

Yeah, that is what I was tackling this very moment.

> diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
> index 8355c3895894..22ec9e275335 100644
> --- a/arch/arm/mm/Kconfig
> +++ b/arch/arm/mm/Kconfig
> @@ -278,7 +278,7 @@ config CPU_ARM1026
>   # SA110
>   config CPU_SA110
>          bool
> -       select CPU_32v3 if ARCH_RPC
> +       select CPU_32v4 if ARCH_RPC

Does that build fully for xconfig or any others which you tried?

>          select CPU_32v4 if !ARCH_RPC
>          select CPU_ABRT_EV4
>          select CPU_CACHE_V4WB
> 

Thanks to all!

Re: [PATCH 0/3] Remove scsi_cmnd.tag

[Hannes Reinecke]

On 8/19/21 9:27 AM, John Garry wrote:
> On 19/08/2021 08:15, Hannes Reinecke wrote:
>> Hey Bart,
>>
>> Thanks for this!
>> Really helpful.
>>
>> Just a tiny wee snag:
>>
>> On 8/19/21 4:41 AM, Bart Van Assche wrote:
>>> On 8/18/21 11:08 AM, John Garry wrote:
>>>> Or maybe you or Bart have a better idea?
>>>
>>> This is how I test compilation of SCSI drivers on a SUSE system (only
>>> the cross-compilation prefix is distro specific):
>>>
>>>      # Acorn RiscPC
>>>      make ARCH=arm xconfig
>>>      # Select the RiscPC architecture (ARCH_RPC)
>>>      make -j9 ARCH=arm CROSS_COMPILE=arm-suse-linux-gnueabi- </dev/null
>>>
>>
>> Acorn RiscPC is ARMv3, which sadly isn't supported anymore with gcc9.
>> So for compilation I had to modify Kconfig to select ARMv4:
>>
> 
> Yeah, that is what I was tackling this very moment.
> 
>> diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
>> index 8355c3895894..22ec9e275335 100644
>> --- a/arch/arm/mm/Kconfig
>> +++ b/arch/arm/mm/Kconfig
>> @@ -278,7 +278,7 @@ config CPU_ARM1026
>>   # SA110
>>   config CPU_SA110
>>          bool
>> -       select CPU_32v3 if ARCH_RPC
>> +       select CPU_32v4 if ARCH_RPC
> 
> Does that build fully for xconfig or any others which you tried?
> 

Yep, xconfig and full build works.

Well.

Would've worked if you hadn't messed up tag handling for acornscsi :-)

Besides: tag handling in acornscsi (and fas216, for that matter) seems 
to be completely broken.

Consider this beauty:

#ifdef CONFIG_SCSI_ACORNSCSI_TAGGED_QUEUE
        /*
         * tagged queueing - allocate a new tag to this command
         */
        if (SCpnt->device->simple_tags) {
            SCpnt->device->current_tag += 1;
            if (SCpnt->device->current_tag == 0)
                SCpnt->device->current_tag = 1;
            SCpnt->tag = SCpnt->device->current_tag;
        } else
#endif

which is broken on _soo many_ counts.
Not only does it try to allocate its own tags, the code also assumes 
that a tag value of '0' indicates that tagged queueing is not active:

static
void acornscsi_abortcmd(AS_Host *host, unsigned char tag)
{
     host->scsi.phase = PHASE_ABORTED;
     sbic_arm_write(host, SBIC_CMND, CMND_ASSERTATN);

     msgqueue_flush(&host->scsi.msgs);
#ifdef CONFIG_SCSI_ACORNSCSI_TAGGED_QUEUE
     if (tag)
         msgqueue_addmsg(&host->scsi.msgs, 2, ABORT_TAG, tag);
     else
#endif
         msgqueue_addmsg(&host->scsi.msgs, 1, ABORT);
}

And, of course, there's the usual confusion about when to check for
sdev->tagged_supported and sdev->simple_tags.

Drop me a note if I can give a hand.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 0/3] Remove scsi_cmnd.tag

[John Garry]

On 19/08/2021 08:50, Hannes Reinecke wrote:
>>>    select CPU_32v4 if ARCH_RPC
>>
>> Does that build fully for xconfig or any others which you tried?
>>
>  > Yep, xconfig and full build works.
> 
> Well.
> 
> Would've worked if you hadn't messed up tag handling for acornscsi :-)
>  > Besides: tag handling in acornscsi (and fas216, for that matter) seems
> to be completely broken.
> 
> Consider this beauty:
> 
> #ifdef CONFIG_SCSI_ACORNSCSI_TAGGED_QUEUE
>         /*
>          * tagged queueing - allocate a new tag to this command
>          */
>         if (SCpnt->device->simple_tags) {
>             SCpnt->device->current_tag += 1;
>             if (SCpnt->device->current_tag == 0)
>                 SCpnt->device->current_tag = 1;
>             SCpnt->tag = SCpnt->device->current_tag;
>         } else
> #endif

So isn't this just using the scsi_cmnd.tag as it own scribble?

> 
> which is broken on _soo many_ counts.
> Not only does it try to allocate its own tags, the code also assumes 
> that a tag value of '0' indicates that tagged queueing is not active:
> 

In case you missed it, Arnd B tried to clear out some old platforms 
earlier this year. With respect to rpc, Russell King apparently still 
uses it and has some SCSI patches:

https://lore.kernel.org/lkml/20210109174357.GB1551@shell.armlinux.org.uk/

I wonder what they are and maybe we can check. Anyway... I'd run any 
changes by him...

> static
> void acornscsi_abortcmd(AS_Host *host, unsigned char tag)
> {
>      host->scsi.phase = PHASE_ABORTED;
>      sbic_arm_write(host, SBIC_CMND, CMND_ASSERTATN);
> 
>      msgqueue_flush(&host->scsi.msgs);
> #ifdef CONFIG_SCSI_ACORNSCSI_TAGGED_QUEUE
>      if (tag)
>          msgqueue_addmsg(&host->scsi.msgs, 2, ABORT_TAG, tag);
>      else
> #endif
>          msgqueue_addmsg(&host->scsi.msgs, 1, ABORT);
> }
> 
> And, of course, there's the usual confusion about when to check for
> sdev->tagged_supported and sdev->simple_tags.
> 
> Drop me a note if I can give a hand.

Thanks! Let's see what happens to the series which you just sent.

Re: [PATCH 0/3] Remove scsi_cmnd.tag

[Martin K. Petersen]

On Fri, 13 Aug 2021 21:49:10 +0800, John Garry wrote:

> There is no need for scsi_cmnd.tag, so remove it.
> 
> Based on next-20210811
> 
> John Garry (3):
>   scsi: wd719: Stop using scsi_cmnd.tag
>   scsi: fnic: Stop setting scsi_cmnd.tag
>   scsi: Remove scsi_cmnd.tag
> 
> [...]

Applied to 5.15/scsi-queue, thanks!

[1/3] scsi: wd719: Stop using scsi_cmnd.tag
      https://git.kernel.org/mkp/scsi/c/e2a1dc571e19
[2/3] scsi: fnic: Stop setting scsi_cmnd.tag
      https://git.kernel.org/mkp/scsi/c/e0aebd25fdd9
[3/3] scsi: Remove scsi_cmnd.tag
      https://git.kernel.org/mkp/scsi/c/4c7b6ea336c1

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/3] Remove scsi_cmnd.tag

[John Garry]

On 24/08/2021 05:03, Martin K. Petersen wrote:
> On Fri, 13 Aug 2021 21:49:10 +0800, John Garry wrote:
> 
>> There is no need for scsi_cmnd.tag, so remove it.
>>
>> Based on next-20210811
>>
>> John Garry (3):
>>    scsi: wd719: Stop using scsi_cmnd.tag
>>    scsi: fnic: Stop setting scsi_cmnd.tag
>>    scsi: Remove scsi_cmnd.tag
>>
>> [...]
> 
> Applied to 5.15/scsi-queue, thanks!
> 
> [1/3] scsi: wd719: Stop using scsi_cmnd.tag
>        https://git.kernel.org/mkp/scsi/c/e2a1dc571e19
> [2/3] scsi: fnic: Stop setting scsi_cmnd.tag
>        https://git.kernel.org/mkp/scsi/c/e0aebd25fdd9
> [3/3] scsi: Remove scsi_cmnd.tag
>        https://git.kernel.org/mkp/scsi/c/4c7b6ea336c1
> 

Thanks, but we still have the issue with the arm drivers.

I'll ping Russell again if he doesn't reply soon.

Hannes also sent a series - that may be the way forward, but need 
Russell involved.

John