Subject: Re: dev/scsipi/ch.c:chattach() fix ? (and approval for commit?)
To: Stoned Elipot <seb@ssr.univ-paris7.fr>
From: Manuel Bouyer <bouyer@antioche.eu.org>
List: tech-kern
Date: 06/15/2004 22:49:08
--gKMricLos+KVdGMg
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Jun 15, 2004 at 04:48:24PM +0200, Stoned Elipot wrote:
> Hi,
> When I updated the box to which my scsi changer is attached from 1.6.2_STABLE
> to 2.0_BETA I've lost the pretty autoconf boot message about its 
> characteristics.
> 
> With straight 2.0_BETA I have:
> ch0 at scsibus1 target 4 lun 0: <QUALSTAR, RLS-4221, 0047> changer removable
> ch0: 0 slots, 0 drives, 0 pickers, 0 portals
> 
> A first chio params after boot gives correct informations expect the current
> picker is somewhat wrong:
> # chio params 
> /dev/ch0: 22 slots, 1 drive, 1 picker
> /dev/ch0: current picker: -65000
> 
> After digging around I've found that the first call to ch_get_params() from
> ch.c:chattach() gathers mostly zero values.
> 
> Reading the diffs of dev/scsipi/ between 1.6 branch and current I tried
> to add a dummy scsipi_test_unit_ready() in chattach(). Such a call was
> previously done in scsiconf.c:scsi_probe_device() and was removed in its
> revision 1.216. That solved my autoconf information gathering problem
> so then things like current picker right after boot is ok.
> 
> I now have:
> ch0 at scsibus1 target 4 lun 0: <QUALSTAR, RLS-4221, 0047> changer removable
> ch0: 22 slots, 1 drive, 1 picker, 0 portals
> 
> Attached is my diff. I'd like to have your comments on it.
> 
> Cheers, Stoned.

> Index: ch.c
> ===================================================================
> RCS file: /cvsroot/src/sys/dev/scsipi/ch.c,v
> retrieving revision 1.59
> diff -u -u -r1.59 ch.c
> --- ch.c	23 Apr 2004 21:52:17 -0000	1.59
> +++ ch.c	15 Jun 2004 12:53:53 -0000
> @@ -225,6 +225,14 @@
>  	}
>  
>  	/*
> +	 * Do a dummy TEST UNIT READY to shake the changer, it helps
> +	 * gathering information about the device for some model.
> +	 */
> +	(void)scsipi_test_unit_ready(sc->sc_periph, XS_CTL_DISCOVERY |
> +	    XS_CTL_IGNORE_ILLEGAL_REQUEST | XS_CTL_IGNORE_NOT_READY |
> +	    XS_CTL_IGNORE_MEDIA_CHANGE);
> +
> +	/*
>  	 * Get information about the device.  Note we can't use
>  	 * interrupts yet.
>  	 */

BTW, this comment is wrong now :)

I think this just hides the problem. We should find why we don't get an error
while the data was not updated. I suspect it's related to a Unit Attention
condition that would be cleared by the scsipi_test_unit_ready(), but I
don't see how it could be a problem.
Can you test the attached patch ? It should emit messages when a command ends
with Unit Attention and has XS_CTL_IGNORE_MEDIA_CHANGE set, and another
message when such a command is restarted.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

--gKMricLos+KVdGMg
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename=diff

Index: scsipi_base.c
===================================================================
RCS file: /cvsroot/src/sys/dev/scsipi/scsipi_base.c,v
retrieving revision 1.105
diff -u -r1.105 scsipi_base.c
--- scsipi_base.c	27 Apr 2004 18:15:37 -0000	1.105
+++ scsipi_base.c	15 Jun 2004 20:46:50 -0000
@@ -947,6 +947,8 @@
 				/* XXX Should reupload any transient state. */
 				(periph->periph_flags &
 				 PERIPH_REMOVABLE) == 0) {
+				scsipi_printaddr(periph);
+				printf("got Unit Attention, returning ERESTART\n");
 				return (ERESTART);
 			}
 			if ((xs->xs_control & XS_CTL_SILENT) != 0)
@@ -1651,6 +1653,8 @@
 
 	s = splbio();
 	if (error == ERESTART) {
+		scsipi_printaddr(periph);
+		printf("restarting command\n");
 		/*
 		 * If we get here, the periph has been thawed and frozen
 		 * again if we had to issue recovery commands.  Alternatively,

--gKMricLos+KVdGMg--