On Sat, 8 Nov 2014, Michael Schmitz wrote:
> > Index: linux/drivers/scsi/atari_NCR5380.c
> > ===================================================================
> > --- linux.orig/drivers/scsi/atari_NCR5380.c 2014-10-27 16:25:36.000000000
> > +1100
> > +++ linux/drivers/scsi/atari_NCR5380.c 2014-10-27 16:25:45.000000000
> > +1100
> > @@ -879,10 +879,10 @@ static void NCR5380_exit(struct Scsi_Hos
> > *
> > */
> > -static int NCR5380_queue_command_lck(struct scsi_cmnd *cmd,
> > - void (*done)(struct scsi_cmnd *))
> > +static int NCR5380_queue_command(struct Scsi_Host *instance,
> > + struct scsi_cmnd *cmd)
> > {
> > - SETUP_HOSTDATA(cmd->device->host);
> > + struct NCR5380_hostdata *hostdata = shost_priv(instance);
> > struct scsi_cmnd *tmp;
> > unsigned long flags;
>
> Nitpick - why did this change set go into this particular patch? Because you
> are converting from NCR5380_queue_command_lck to NCR5380_queue_command?
That's right.
The SETUP_HOSTDATA macro does not appear in NCR5380.c, it is peculiar to
atari_NCR5380.c. Like so much pre-processor abuse in these drivers, this
example is undesirable as it harms readability, whereas the shost_priv()
convention is common and readily understood.
So the uniform adoption of,
struct NCR5380_hostdata *hostdata = shost_priv(instance);
is the purpose of a different patch (unsent). I have more unsent patches
to fix up other weird macros in atari_NCR5380.c like HOSTDATA, H_NO, etc.
It's instructive to compare this change with the use of the
NCR5380_local_declare() macro in NCR5380.c, which was long ago dropped
from atari_NCR5380.c (and quite rightly so).
>
> > @@ -893,7 +893,7 @@ static int NCR5380_queue_command_lck(str
> > printk(KERN_NOTICE "scsi%d: WRITE attempted with NO_WRITE debugging flag
> > set\n",
> > H_NO(cmd));
> > cmd->result = (DID_ERROR << 16);
> > - done(cmd);
> > + cmd->scsi_done(cmd);
> > return 0;
> > }
> > #endif /* (NDEBUG & NDEBUG_NO_WRITE) */
> > @@ -904,8 +904,6 @@ static int NCR5380_queue_command_lck(str
> > */
> >
> > SET_NEXT(cmd, NULL);
> > - cmd->scsi_done = done;
> > -
> > cmd->result = 0;
> >
> > /*
>
> Ditto for these two.
Again, it follows from the differences in the formal parameters between
NCR5380_queue_command_lck() and NCR5380_queue_command().
>
> > @@ -915,7 +913,6 @@ static int NCR5380_queue_command_lck(str
> > * sense data is only guaranteed to be valid while the condition exists.
> > */
> > - local_irq_save(flags);
> > /* ++guenther: now that the issue queue is being set up, we can lock
> > ST-DMA.
> > * Otherwise a running NCR5380_main may steal the lock.
> > * Lock before actually inserting due to fairness reasons explained in
> > @@ -928,11 +925,13 @@ static int NCR5380_queue_command_lck(str
> > * because also a timer int can trigger an abort or reset, which would
> > * alter queues and touch the lock.
> > */
> > - if (!IS_A_TT()) {
> > - /* perhaps stop command timer here */
> > - falcon_get_lock();
> > - /* perhaps restart command timer here */
> > - }
> > + /* perhaps stop command timer here */
> > + if (!falcon_get_lock())
> > + return SCSI_MLQUEUE_HOST_BUSY;
> > + /* perhaps restart command timer here */
> > +
>
> The comments about stopping and restarting the command timer can be
> removed. In 2.4 kernels, the driver would tweak the timers and wait on
> the lock unconditionally, Can't ne done anymore, for so many reasons.
Well, you sent an acked-by for this patch, so I'm a bit confused. Do you
want me to re-spin it?
--
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
