diff mbox series

[v2,3/6] esp_scsi: Grant disconnect privilege for untagged commands

Message ID 91b9ff9fc19ba40e3a2562171c8cd78a2ce7dc79.1539497520.git.fthain@telegraphics.com.au (mailing list archive)
State Changes Requested
Headers show
Series mac_esp, zorro_esp, esp_scsi: Various improvements | expand

Commit Message

Finn Thain Oct. 14, 2018, 6:12 a.m. UTC 
A SCSI device is not granted disconnect privilege by an esp_scsi host
unless that device has its simple_tags flag set. However, a device may
support disconnect/reselect and not support command queueing. Allow
these devices to disconnect and improve bus utilization.

Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Tested-by: Michael Schmitz <schmitzmic@gmail.com>
---
 drivers/scsi/esp_scsi.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

Christoph Hellwig Oct. 14, 2018, 3:47 p.m. UTC | #1 
> +	*p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);

I think lp should always be non-NULL here.
Michael Schmitz Oct. 14, 2018, 8:33 p.m. UTC | #2 
On 15/10/18 04:47, Christoph Hellwig wrote:

>> +	*p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);
> I think lp should always be non-NULL here.

That indeed appears to be the case these days.

So we can't rely on !lp to detect when probing the bus any longer. What 
else would be available? Do commands used for device probing also have a 
tag these days by default?

Do we really need to make that distinction?

Cheers,

     Michael
Finn Thain Oct. 14, 2018, 11:04 p.m. UTC | #3 
On Mon, 15 Oct 2018, Michael Schmitz wrote:

> 
> Do we really need to make that distinction?
> 

esp_reconnect() dereferences lp, so !lp has to inhibit disconnection.

At least, that's my reading. I can't see any other reason for the lp 
conditional.

--
Finn Thain Oct. 14, 2018, 11:13 p.m. UTC | #4 
On Sun, 14 Oct 2018, Christoph Hellwig wrote:

> > +	*p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);
> 
> I think lp should always be non-NULL here.
> 

It's not clear from Documentation/scsi/scsi_mid_low_api.txt, but I guess 
that's true for scanning of targets.

Is it true in general that queuecommand for a device will not occur before 
its slave_alloc and not after its slave_destroy invocation?

(I'm thinking of queuecommand via the other command submission paths, like 
ioctl.)

--
Finn Thain Oct. 15, 2018, 2:45 a.m. UTC | #5 
On Mon, 15 Oct 2018, Finn Thain wrote:

> On Sun, 14 Oct 2018, Christoph Hellwig wrote:
> 
> > > +	*p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);
> > 
> > I think lp should always be non-NULL here.
> > 
> 
> It's not clear from Documentation/scsi/scsi_mid_low_api.txt, but I guess 
> that's true for scanning of targets.
> 
> Is it true in general that queuecommand for a device will not occur 
> before its slave_alloc and not after its slave_destroy invocation?
> 
> (I'm thinking of queuecommand via the other command submission paths, 
> like ioctl.)
> 

Nevermind. From a closer reading of the Documentation, I see that it is 
true in general.

--
Christoph Hellwig Oct. 15, 2018, 5:52 a.m. UTC | #6 
On Mon, Oct 15, 2018 at 10:13:37AM +1100, Finn Thain wrote:
> On Sun, 14 Oct 2018, Christoph Hellwig wrote:
> 
> > > +	*p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);
> > 
> > I think lp should always be non-NULL here.
> > 
> 
> It's not clear from Documentation/scsi/scsi_mid_low_api.txt, but I guess 
> that's true for scanning of targets.
> 
> Is it true in general that queuecommand for a device will not occur before 
> its slave_alloc and not after its slave_destroy invocation?

Yes.
diff mbox series

Patch

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 9e5d3f7d29ae..b7c41aa9927c 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -800,13 +800,9 @@  static void esp_maybe_execute_command(struct esp *esp)
 
 build_identify:
 	/* If we don't have a lun-data struct yet, we're probing
-	 * so do not disconnect.  Also, do not disconnect unless
-	 * we have a tag on this command.
+	 * so do not disconnect.
 	 */
-	if (lp && (tp->flags & ESP_TGT_DISCONNECT) && ent->tag[0])
-		*p++ = IDENTIFY(1, lun);
-	else
-		*p++ = IDENTIFY(0, lun);
+	*p++ = IDENTIFY(lp && (tp->flags & ESP_TGT_DISCONNECT), lun);
 
 	if (ent->tag[0] && esp->rev == ESP100) {
 		/* ESP100 lacks select w/atn3 command, use select