Re: [PATCH v3 22/77] ncr5380: Eliminate selecting state
On 12/22/2015 02:18 AM, Finn Thain wrote: Linux v2.1.105 changed the algorithm for polling for the BSY signal in NCR5380_select() and NCR5380_main(). Presently, this code has a bug. Back then, NCR5380_set_timer(hostdata, 1) meant reschedule main() after sleeping for 10 ms. Repeated 25 times this provided the recommended 250 ms selection time-out delay. This got broken when HZ became configurable. We could fix this but there's no need to reschedule the main loop. This BSY polling presently happens when the NCR5380_main() work queue item calls NCR5380_select(), which in turn schedules NCR5380_main(), which calls NCR5380_select() again, and so on. This algorithm is a deviation from the simpler one in atari_NCR5380.c. The extra complexity and state is pointless. There's no reason to stop selection half-way and return to to the main loop when the main loop can do nothing useful until selection completes. So just poll for BSY. We can sleep while polling now that we have a suitable workqueue. Signed-off-by: Finn Thain Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3 22/77] ncr5380: Eliminate selecting state
Linux v2.1.105 changed the algorithm for polling for the BSY signal in NCR5380_select() and NCR5380_main(). Presently, this code has a bug. Back then, NCR5380_set_timer(hostdata, 1) meant reschedule main() after sleeping for 10 ms. Repeated 25 times this provided the recommended 250 ms selection time-out delay. This got broken when HZ became configurable. We could fix this but there's no need to reschedule the main loop. This BSY polling presently happens when the NCR5380_main() work queue item calls NCR5380_select(), which in turn schedules NCR5380_main(), which calls NCR5380_select() again, and so on. This algorithm is a deviation from the simpler one in atari_NCR5380.c. The extra complexity and state is pointless. There's no reason to stop selection half-way and return to to the main loop when the main loop can do nothing useful until selection completes. So just poll for BSY. We can sleep while polling now that we have a suitable workqueue. Signed-off-by: Finn Thain --- drivers/scsi/NCR5380.c | 74 --- drivers/scsi/NCR5380.h |2 - drivers/scsi/atari_NCR5380.c | 49 3 files changed, 29 insertions(+), 96 deletions(-) Index: linux/drivers/scsi/NCR5380.c === --- linux.orig/drivers/scsi/NCR5380.c 2015-12-22 12:16:00.0 +1100 +++ linux/drivers/scsi/NCR5380.c2015-12-22 12:16:02.0 +1100 @@ -988,7 +988,7 @@ static void NCR5380_main(struct work_str do { /* Lock held here */ done = 1; - if (!hostdata->connected && !hostdata->selecting) { + if (!hostdata->connected) { dprintk(NDEBUG_MAIN, "scsi%d : not connected\n", instance->host_no); /* * Search through the issue_queue for a command destined @@ -1018,9 +1018,6 @@ static void NCR5380_main(struct work_str */ dprintk(NDEBUG_MAIN|NDEBUG_QUEUES, "scsi%d : main() : command for target %d lun %llu removed from issue_queue\n", instance->host_no, tmp->device->id, tmp->device->lun); - hostdata->selecting = NULL; - /* RvC: have to preset this to indicate a new command is being performed */ - /* * REQUEST SENSE commands are issued without tagged * queueing, even on SCSI-II devices because the @@ -1038,26 +1035,13 @@ static void NCR5380_main(struct work_str done = 0; dprintk(NDEBUG_MAIN|NDEBUG_QUEUES, "scsi%d : main(): select() failed, returned to issue_queue\n", instance->host_no); } - if (hostdata->connected || - hostdata->selecting) + if (hostdata->connected) break; /* lock held here still */ } /* if target/lun is not busy */ } /* for */ /* exited locked */ } /* if (!hostdata->connected) */ - if (hostdata->selecting) { - tmp = (struct scsi_cmnd *) hostdata->selecting; - /* Selection will drop and retake the lock */ - if (!NCR5380_select(instance, tmp)) { - /* OK or bad target */ - } else { - LIST(tmp, hostdata->issue_queue); - tmp->host_scribble = (unsigned char *) hostdata->issue_queue; - hostdata->issue_queue = tmp; - done = 0; - } - } /* if hostdata->selecting */ if (hostdata->connected #ifdef REAL_DMA && !hostdata->dmalen @@ -1176,7 +1160,6 @@ static irqreturn_t NCR5380_intr(int dumm * Returns : -1 if selection failed but should be retried. * 0 if selection failed and should not be retried. * 0 if selection succeeded completely (hostdata->connected == cmd). - * 0 if selection in progress (hostdata->selecting == cmd). * * Side effects : * If bus busy, arbitration failed, etc, NCR5380_select() will exit @@ -1200,13 +1183,8 @@ static int NCR5380_select(struct Scsi_Ho unsigned char tmp[3], phase; unsigned char *data; int len; - unsigned long timeout; - unsigned char value; int err; - if
[PATCH v3 22/77] ncr5380: Eliminate selecting state
Linux v2.1.105 changed the algorithm for polling for the BSY signal in NCR5380_select() and NCR5380_main(). Presently, this code has a bug. Back then, NCR5380_set_timer(hostdata, 1) meant reschedule main() after sleeping for 10 ms. Repeated 25 times this provided the recommended 250 ms selection time-out delay. This got broken when HZ became configurable. We could fix this but there's no need to reschedule the main loop. This BSY polling presently happens when the NCR5380_main() work queue item calls NCR5380_select(), which in turn schedules NCR5380_main(), which calls NCR5380_select() again, and so on. This algorithm is a deviation from the simpler one in atari_NCR5380.c. The extra complexity and state is pointless. There's no reason to stop selection half-way and return to to the main loop when the main loop can do nothing useful until selection completes. So just poll for BSY. We can sleep while polling now that we have a suitable workqueue. Signed-off-by: Finn Thain--- drivers/scsi/NCR5380.c | 74 --- drivers/scsi/NCR5380.h |2 - drivers/scsi/atari_NCR5380.c | 49 3 files changed, 29 insertions(+), 96 deletions(-) Index: linux/drivers/scsi/NCR5380.c === --- linux.orig/drivers/scsi/NCR5380.c 2015-12-22 12:16:00.0 +1100 +++ linux/drivers/scsi/NCR5380.c2015-12-22 12:16:02.0 +1100 @@ -988,7 +988,7 @@ static void NCR5380_main(struct work_str do { /* Lock held here */ done = 1; - if (!hostdata->connected && !hostdata->selecting) { + if (!hostdata->connected) { dprintk(NDEBUG_MAIN, "scsi%d : not connected\n", instance->host_no); /* * Search through the issue_queue for a command destined @@ -1018,9 +1018,6 @@ static void NCR5380_main(struct work_str */ dprintk(NDEBUG_MAIN|NDEBUG_QUEUES, "scsi%d : main() : command for target %d lun %llu removed from issue_queue\n", instance->host_no, tmp->device->id, tmp->device->lun); - hostdata->selecting = NULL; - /* RvC: have to preset this to indicate a new command is being performed */ - /* * REQUEST SENSE commands are issued without tagged * queueing, even on SCSI-II devices because the @@ -1038,26 +1035,13 @@ static void NCR5380_main(struct work_str done = 0; dprintk(NDEBUG_MAIN|NDEBUG_QUEUES, "scsi%d : main(): select() failed, returned to issue_queue\n", instance->host_no); } - if (hostdata->connected || - hostdata->selecting) + if (hostdata->connected) break; /* lock held here still */ } /* if target/lun is not busy */ } /* for */ /* exited locked */ } /* if (!hostdata->connected) */ - if (hostdata->selecting) { - tmp = (struct scsi_cmnd *) hostdata->selecting; - /* Selection will drop and retake the lock */ - if (!NCR5380_select(instance, tmp)) { - /* OK or bad target */ - } else { - LIST(tmp, hostdata->issue_queue); - tmp->host_scribble = (unsigned char *) hostdata->issue_queue; - hostdata->issue_queue = tmp; - done = 0; - } - } /* if hostdata->selecting */ if (hostdata->connected #ifdef REAL_DMA && !hostdata->dmalen @@ -1176,7 +1160,6 @@ static irqreturn_t NCR5380_intr(int dumm * Returns : -1 if selection failed but should be retried. * 0 if selection failed and should not be retried. * 0 if selection succeeded completely (hostdata->connected == cmd). - * 0 if selection in progress (hostdata->selecting == cmd). * * Side effects : * If bus busy, arbitration failed, etc, NCR5380_select() will exit @@ -1200,13 +1183,8 @@ static int NCR5380_select(struct Scsi_Ho unsigned char tmp[3], phase; unsigned char *data; int len; - unsigned long timeout; - unsigned char value; int err;
Re: [PATCH v3 22/77] ncr5380: Eliminate selecting state
On 12/22/2015 02:18 AM, Finn Thain wrote: Linux v2.1.105 changed the algorithm for polling for the BSY signal in NCR5380_select() and NCR5380_main(). Presently, this code has a bug. Back then, NCR5380_set_timer(hostdata, 1) meant reschedule main() after sleeping for 10 ms. Repeated 25 times this provided the recommended 250 ms selection time-out delay. This got broken when HZ became configurable. We could fix this but there's no need to reschedule the main loop. This BSY polling presently happens when the NCR5380_main() work queue item calls NCR5380_select(), which in turn schedules NCR5380_main(), which calls NCR5380_select() again, and so on. This algorithm is a deviation from the simpler one in atari_NCR5380.c. The extra complexity and state is pointless. There's no reason to stop selection half-way and return to to the main loop when the main loop can do nothing useful until selection completes. So just poll for BSY. We can sleep while polling now that we have a suitable workqueue. Signed-off-by: Finn ThainReviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/