Re: [PATCH v2 19/44] fnic: Stop using the SCSI pointer

[Bart Van Assche <bvanassche@acm.org>]

On 2/8/22 23:56, Hannes Reinecke wrote:
> On 2/8/22 18:24, Bart Van Assche wrote:
>> Set .cmd_size in the SCSI host template instead of using the SCSI pointer
>> from struct scsi_cmnd. This patch prepares for removal of the SCSI 
>> pointer
>> from struct scsi_cmnd.
>>
>> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>   drivers/scsi/fnic/fnic.h      |  27 +++-
>>   drivers/scsi/fnic/fnic_main.c |   1 +
>>   drivers/scsi/fnic/fnic_scsi.c | 289 +++++++++++++++++-----------------
>>   3 files changed, 163 insertions(+), 154 deletions(-)
>>
>> diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
>> index b95d0063dedb..aa07189fb5fb 100644
>> --- a/drivers/scsi/fnic/fnic.h
>> +++ b/drivers/scsi/fnic/fnic.h
>> @@ -89,15 +89,28 @@
>>   #define FNIC_DEV_RST_ABTS_PENDING       BIT(21)
>>   /*
>> - * Usage of the scsi_cmnd scratchpad.
>> + * fnic private data per SCSI command.
>>    * These fields are locked by the hashed io_req_lock.
>>    */
>> -#define CMD_SP(Cmnd)        ((Cmnd)->SCp.ptr)
>> -#define CMD_STATE(Cmnd)        ((Cmnd)->SCp.phase)
>> -#define CMD_ABTS_STATUS(Cmnd)    ((Cmnd)->SCp.Message)
>> -#define CMD_LR_STATUS(Cmnd)    ((Cmnd)->SCp.have_data_in)
>> -#define CMD_TAG(Cmnd)           ((Cmnd)->SCp.sent_command)
>> -#define CMD_FLAGS(Cmnd)         ((Cmnd)->SCp.Status)
>> +struct fnic_cmd_priv {
>> +    struct fnic_io_req *io_req;
>> +    enum fnic_ioreq_state state;
>> +    u32 flags;
>> +    u16 abts_status;
>> +    u16 lr_status;
>> +};
>> +
>> +static inline struct fnic_cmd_priv *fnic_priv(struct scsi_cmnd *cmd)
>> +{
>> +    return scsi_cmd_priv(cmd);
>> +}
>> +
>> +static inline u64 fnic_flags_and_state(struct scsi_cmnd *cmd)
>> +{
>> +    struct fnic_cmd_priv *fcmd = fnic_priv(cmd);
>> +
>> +    return ((u64)fcmd->flags << 32) | fcmd->state;
>> +}
>>   #define FCPIO_INVALID_CODE 0x100 /* hdr_status value unused by 
>> firmware */
>> diff --git a/drivers/scsi/fnic/fnic_main.c 
>> b/drivers/scsi/fnic/fnic_main.c
>> index 44dbaa662d94..9161bd2fd421 100644
>> --- a/drivers/scsi/fnic/fnic_main.c
>> +++ b/drivers/scsi/fnic/fnic_main.c
>> @@ -124,6 +124,7 @@ static struct scsi_host_template 
>> fnic_host_template = {
>>       .max_sectors = 0xffff,
>>       .shost_groups = fnic_host_groups,
>>       .track_queue_depth = 1,
>> +    .cmd_size = sizeof(struct fnic_cmd_priv),
>>   };
>>   static void
>> diff --git a/drivers/scsi/fnic/fnic_scsi.c 
>> b/drivers/scsi/fnic/fnic_scsi.c
>> index 549754245f7a..3c00e5b88350 100644
>> --- a/drivers/scsi/fnic/fnic_scsi.c
>> +++ b/drivers/scsi/fnic/fnic_scsi.c
>> @@ -497,8 +497,8 @@ static int fnic_queuecommand_lck(struct scsi_cmnd 
>> *sc)
>>        * caller disabling them.
>>        */
>>       spin_unlock(lp->host->host_lock);
>> -    CMD_STATE(sc) = FNIC_IOREQ_NOT_INITED;
>> -    CMD_FLAGS(sc) = FNIC_NO_FLAGS;
>> +    fnic_priv(sc)->state = FNIC_IOREQ_NOT_INITED;
>> +    fnic_priv(sc)->flags = FNIC_NO_FLAGS;
> Why not keep the macros?
> Would be less churn with the driver, no?

Keeping the macros would indeed make this patch smaller. My opinion is 
that the CMD_STATE() and CMD_FLAGS() macro definitions are too short to 
keep them and that expanding these macros makes the driver code easier 
to read.

Thanks,

Bart.