aic79xx: fix up transport settings

PATCH In review James Bottomley 2 participants 2 messages Aug 3, 2005 – Aug 4, 2005

Subsystems (2)

SCSI AIC7XXX / AIC79XX SCSI driver

lore

Share & tools

lore

James Bottomley

author

2005-08-03 18:25 UTC #0 lore raw

PATCH 1/1 aic79xx: fix up transport settings

There's a slight problem in the way you've done the transport parameters; reading from the variables actually produces the current settings, not the ones you just set (and there's usually a lag because devices don't renegotiate until the next command goes over the bus). If you set the bit immediately, you get into the situation where the transport parameters report something as being set even if the drive cannot support it.

I patched the driver to do it this way and also corrected a panic in the proc routines.

James

drivers/scsi/aic7xxx/aic79xx_osm.c +9 ~~-30~~

@@ -1624,7 +1624,11 @@ ahd_send_async(struct ahd_softc *ahd, ch
 		target_ppr_options =
 			(spi_dt(starget) ? MSG_EXT_PPR_DT_REQ : 0)
 			+ (spi_qas(starget) ? MSG_EXT_PPR_QAS_REQ : 0)
-			+ (spi_iu(starget) ?  MSG_EXT_PPR_IU_REQ : 0);
+			+ (spi_iu(starget) ?  MSG_EXT_PPR_IU_REQ : 0)
+			+ (spi_rd_strm(starget) ? MSG_EXT_PPR_RD_STRM : 0)
+			+ (spi_pcomp_en(starget) ? MSG_EXT_PPR_PCOMP_EN : 0)
+			+ (spi_rti(starget) ? MSG_EXT_PPR_RTI : 0)
+			+ (spi_wr_flow(starget) ? MSG_EXT_PPR_WR_FLOW : 0);
 
 		if (tinfo->curr.period == spi_period(starget)
 		    && tinfo->curr.width == spi_width(starget)

@@ -1639,6 +1643,10 @@ ahd_send_async(struct ahd_softc *ahd, ch
 		spi_dt(starget) = tinfo->curr.ppr_options & MSG_EXT_PPR_DT_REQ ? 1 : 0;
 		spi_qas(starget) = tinfo->curr.ppr_options & MSG_EXT_PPR_QAS_REQ ? 1 : 0;
 		spi_iu(starget) = tinfo->curr.ppr_options & MSG_EXT_PPR_IU_REQ ? 1 : 0;
+		spi_rd_strm(starget) = tinfo->curr.ppr_options & MSG_EXT_PPR_RD_STRM ? 1 : 0;
+		spi_pcomp_en(starget) =  tinfo->curr.ppr_options & MSG_EXT_PPR_PCOMP_EN ? 1 : 0;
+		spi_rti(starget) =  tinfo->curr.ppr_options &  MSG_EXT_PPR_RTI ? 1 : 0;
+		spi_wr_flow(starget) = tinfo->curr.ppr_options & MSG_EXT_PPR_WR_FLOW ? 1 : 0;
 		spi_display_xfer_agreement(starget);
 		break;
 	}

@@ -2318,18 +2326,6 @@ done:
 
 static void ahd_linux_exit(void);
 
-static void ahd_linux_set_xferflags(struct scsi_target *starget, unsigned int ppr_options, unsigned int period)
-{
-	spi_qas(starget) = (ppr_options & MSG_EXT_PPR_QAS_REQ)? 1 : 0;
-	spi_dt(starget) = (ppr_options & MSG_EXT_PPR_DT_REQ)? 1 : 0;
-	spi_iu(starget) = (ppr_options & MSG_EXT_PPR_IU_REQ) ? 1 : 0;
-	spi_rd_strm(starget) = (ppr_options & MSG_EXT_PPR_RD_STRM) ? 1 : 0;
-	spi_wr_flow(starget) = (ppr_options & MSG_EXT_PPR_WR_FLOW) ? 1 : 0;
-	spi_pcomp_en(starget) = (ppr_options & MSG_EXT_PPR_PCOMP_EN) ? 1 : 0;
-	spi_rti(starget) = (ppr_options & MSG_EXT_PPR_RTI) ? 1 : 0;
-	spi_period(starget) = period;
-}
-
 static void ahd_linux_set_width(struct scsi_target *starget, int width)
 {
 	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);

@@ -2388,8 +2384,6 @@ static void ahd_linux_set_period(struct 
 	ahd_find_syncrate(ahd, &period, &ppr_options,
 			  dt ? AHD_SYNCRATE_MAX : AHD_SYNCRATE_ULTRA2);
 
-	ahd_linux_set_xferflags(starget, ppr_options, period);
-
 	ahd_lock(ahd, &flags);
 	ahd_set_syncrate(ahd, &devinfo, period, offset,
 			 ppr_options, AHD_TRANS_GOAL, FALSE);

@@ -2424,7 +2418,6 @@ static void ahd_linux_set_offset(struct 
 		ahd_find_syncrate(ahd, &period, &ppr_options, 
 				  dt ? AHD_SYNCRATE_MAX : AHD_SYNCRATE_ULTRA2);
 	}
-	ahd_linux_set_xferflags(starget, ppr_options, period);
 
 	ahd_lock(ahd, &flags);
 	ahd_set_syncrate(ahd, &devinfo, period, offset, ppr_options,

@@ -2467,8 +2460,6 @@ static void ahd_linux_set_dt(struct scsi
 	ahd_find_syncrate(ahd, &period, &ppr_options,
 			  dt ? AHD_SYNCRATE_MAX : AHD_SYNCRATE_ULTRA2);
 
-	ahd_linux_set_xferflags(starget, ppr_options, period);
-
 	ahd_lock(ahd, &flags);
 	ahd_set_syncrate(ahd, &devinfo, period, tinfo->goal.offset,
 			 ppr_options, AHD_TRANS_GOAL, FALSE);

@@ -2508,8 +2499,6 @@ static void ahd_linux_set_qas(struct scs
 	ahd_find_syncrate(ahd, &period, &ppr_options,
 			  dt ? AHD_SYNCRATE_MAX : AHD_SYNCRATE_ULTRA2);
 
-	spi_qas(starget) = (ppr_options & MSG_EXT_PPR_QAS_REQ)? 1 : 0;
-
 	ahd_lock(ahd, &flags);
 	ahd_set_syncrate(ahd, &devinfo, period, tinfo->goal.offset,
 			 ppr_options, AHD_TRANS_GOAL, FALSE);

@@ -2550,8 +2539,6 @@ static void ahd_linux_set_iu(struct scsi
 	ahd_find_syncrate(ahd, &period, &ppr_options,
 			  dt ? AHD_SYNCRATE_MAX : AHD_SYNCRATE_ULTRA2);
 
-	ahd_linux_set_xferflags(starget, ppr_options, period);
-
 	ahd_lock(ahd, &flags);
 	ahd_set_syncrate(ahd, &devinfo, period, tinfo->goal.offset,
 			 ppr_options, AHD_TRANS_GOAL, FALSE);

@@ -2588,8 +2575,6 @@ static void ahd_linux_set_rd_strm(struct
 	ahd_find_syncrate(ahd, &period, &ppr_options,
 			  dt ? AHD_SYNCRATE_MAX : AHD_SYNCRATE_ULTRA2);
 
-	spi_rd_strm(starget) = (ppr_options & MSG_EXT_PPR_RD_STRM) ? 1 : 0;
-
 	ahd_lock(ahd, &flags);
 	ahd_set_syncrate(ahd, &devinfo, period, tinfo->goal.offset,
 			 ppr_options, AHD_TRANS_GOAL, FALSE);

@@ -2626,8 +2611,6 @@ static void ahd_linux_set_wr_flow(struct
 	ahd_find_syncrate(ahd, &period, &ppr_options,
 			  dt ? AHD_SYNCRATE_MAX : AHD_SYNCRATE_ULTRA2);
 
-	spi_wr_flow(starget) = (ppr_options & MSG_EXT_PPR_WR_FLOW) ? 1 : 0;
-
 	ahd_lock(ahd, &flags);
 	ahd_set_syncrate(ahd, &devinfo, period, tinfo->goal.offset,
 			 ppr_options, AHD_TRANS_GOAL, FALSE);

@@ -2672,8 +2655,6 @@ static void ahd_linux_set_rti(struct scs
 	ahd_find_syncrate(ahd, &period, &ppr_options,
 			  dt ? AHD_SYNCRATE_MAX : AHD_SYNCRATE_ULTRA2);
 
-	spi_rti(starget) = (ppr_options & MSG_EXT_PPR_RTI) ? 1 : 0;
-
 	ahd_lock(ahd, &flags);
 	ahd_set_syncrate(ahd, &devinfo, period, tinfo->goal.offset,
 			 ppr_options, AHD_TRANS_GOAL, FALSE);

@@ -2710,8 +2691,6 @@ static void ahd_linux_set_pcomp_en(struc
 	ahd_find_syncrate(ahd, &period, &ppr_options,
 			  dt ? AHD_SYNCRATE_MAX : AHD_SYNCRATE_ULTRA2);
 
-	spi_pcomp_en(starget) = (ppr_options & MSG_EXT_PPR_PCOMP_EN) ? 1 : 0;
-
 	ahd_lock(ahd, &flags);
 	ahd_set_syncrate(ahd, &devinfo, period, tinfo->goal.offset,
 			 ppr_options, AHD_TRANS_GOAL, FALSE);

drivers/scsi/aic7xxx/aic79xx_proc.c +2 -2

@@ -178,9 +178,9 @@ ahd_dump_target_state(struct ahd_softc *
 	copy_info(info, "\tUser: ");
 	ahd_format_transinfo(info, &tinfo->user);
 	starget = ahd->platform_data->starget[target_offset];
-	targ = scsi_transport_target_data(starget);
-	if (targ == NULL)
+	if (starget == NULL)
 		return;
+	targ = scsi_transport_target_data(starget);
 
 	copy_info(info, "\tGoal: ");
 	ahd_format_transinfo(info, &tinfo->goal);

Hannes Reinecke

2005-08-04 06:58 UTC #1 lore raw

Quoted 8 lines · James Bottomley

There's a slight problem in the way you've done the transport parameters; reading from the variables actually produces the current settings, not the ones you just set (and there's usually a lag because devices don't renegotiate until the next command goes over the bus). If you set the bit immediately, you get into the situation where the transport parameters report something as being set even if the drive cannot support it.

Well, as usual you're correct. I wasn't quite sure as to _when_ the transport class parameter will be set, as they appearently haven't been set correctly (this was whilst chasing down the ST/DT setting issue). But yours if of course the correct way of doing things.

Quoted 3 lines

I patched the driver to do it this way and also corrected a panic in the proc routines.

THX. Didn't even know there was a proc panic :-).

Cheers,

Hannes

Signature · 9 lines

Dr. Hannes Reinecke			[email protected]
SuSE Linux Products GmbH		S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html