Re: [PATCH 05/44] NCR5380: Move the SCSI pointer to private command data

[Finn Thain <fthain@xxxxxxxxxxxxxx>]

On Fri, 28 Jan 2022, Bart Van Assche wrote:

> diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
> index 845bd2423e66..adaf131aea4d 100644
> --- a/drivers/scsi/NCR5380.h
> +++ b/drivers/scsi/NCR5380.h
> @@ -227,9 +227,17 @@ struct NCR5380_hostdata {
>  };
>  
>  struct NCR5380_cmd {
> +	struct scsi_pointer scsi_pointer;
>  	struct list_head list;
>  };
>  
> +static inline struct scsi_pointer *NCR5380_scsi_pointer(struct scsi_cmnd *cmd)
> +{
> +	struct NCR5380_cmd *ncmd = scsi_cmd_priv(cmd);
> +
> +	return &ncmd->scsi_pointer;
> +}
> +
>  #define NCR5380_PIO_CHUNK_SIZE		256
>  
>  /* Time limit (ms) to poll registers when IRQs are disabled, e.g. during PDMA */

I think this patch series is a good idea but poorly executed.

Firstly, scsi_pointer has long been the name of a struct. It's confusing 
to see that name re-used for variables and functions. Moreover, it was 
always a lousy name (for anything).

Regarding code style, this is legacy code i.e. it pre-dates the ban on 
mixed letter case. (I'm using the word legacy after the dictionary 
definition and not as a kind of weasel word intended to mean deprecated.)

Mixed case names like "BAZ5000_cmd" would be frowned upon for new code but 
this is not new code. So why not just use the name SCp for variables which 
serve the same purpose that the SCp struct did?

IOW, I would prefer to read the following, because SCp presumably means 
"Scsi Command Private data" whereas "scsi_pointer" means nothing to me.

--- a/drivers/scsi/NCR5380.h
+++ b/drivers/scsi/NCR5380.h
@@ -227,9 +227,17 @@ struct NCR5380_hostdata {
 };
   
 struct NCR5380_cmd {
+     struct scsi_pointer SCp;
      struct list_head list;
 };
   
+static inline struct scsi_pointer *NCR5380_to_SCp(struct scsi_cmnd *cmd)
+{
+     struct NCR5380_cmd *ncmd = scsi_cmd_priv(cmd);
+     
+     return &ncmd->SCp;
+}
+ 
 #define NCR5380_PIO_CHUNK_SIZE               256
 
 /* Time limit (ms) to poll registers when IRQs are disabled, e.g. during PDMA */
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index e9d0d99abc86..61fd3244a4ce 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -538,7 +538,8 @@ static int falcon_classify_cmd(struct scsi_cmnd *cmd)
 static int atari_scsi_dma_xfer_len(struct NCR5380_hostdata *hostdata,
                                    struct scsi_cmnd *cmd)
 {
-       int wanted_len = cmd->SCp.this_residual;
+       struct scsi_pointer *SCp = NCR5380_to_SCp(cmd);
+       int wanted_len = SCp->this_residual;
        int possible_len, limit;
 
        if (wanted_len < DMA_MIN_SIZE)
@@ -610,7 +611,8 @@ static int atari_scsi_dma_xfer_len(struct NCR5380_hostdata *hostdata,
        }
 
        /* Last step: apply the hard limit on DMA transfers */
-       limit = (atari_dma_buffer && !STRAM_ADDR(virt_to_phys(cmd->SCp.ptr))) ?
+       limit = (atari_dma_buffer && !STRAM_ADDR(virt_to_phys(SCp->ptr))) ?
                    STRAM_BUFFER_SIZE : 255*512;
        if (possible_len > limit)
                possible_len = limit;