[PATCH 0/5] Fix mpt3sas driver sparse warnings

[PATCH 0/5] Fix mpt3sas driver sparse warnings

[Damien Le Moal]

This series fix (remove) all sparse warnings generated when compiling
the mpt3sas driver. All warnings are related to __iomem access and
endianness.

The series was tested on top of Martin's 5.18/scsi-staging branch with a
9400-8i HBA with direct attached iSAS and SATA drives. The fixes need
careful review by the maintainers as there is no documentation clearly
explaning the proper endianness of the values touched.

Damien Le Moal (5):
  scsi: mpt3sas: fix Mpi2SCSITaskManagementRequest_t TaskMID handling
  scsi: mpt3sas: Fix writel() use
  scsi: mpt3sas: fix ioc->base_readl() use
  scsi: mpt3sas: fix event callback log_code value handling
  scsi: mpt3sas: fix adapter replyPostRegisterIndex handling

 drivers/scsi/mpt3sas/mpi/mpi2_init.h |  2 +-
 drivers/scsi/mpt3sas/mpt3sas_base.c  | 60 ++++++++++++++++------------
 drivers/scsi/mpt3sas/mpt3sas_ctl.c   |  3 +-
 drivers/scsi/mpt3sas/mpt3sas_scsih.c |  6 +--
 4 files changed, 40 insertions(+), 31 deletions(-)

-- 
2.35.1

[PATCH 1/5] scsi: mpt3sas: fix Mpi2SCSITaskManagementRequest_t TaskMID handling

[Damien Le Moal]

The TaskMID field of sturtc Mpi2SCSITaskManagementRequest_t seems to be
a 16-bits little endian value but is not declared as such, causing
sparse to generate warnings. Change this field declaration to __le16 and
fix a test in _ctl_set_task_mid() to avoid sparse warnings.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/mpt3sas/mpi/mpi2_init.h | 2 +-
 drivers/scsi/mpt3sas/mpt3sas_ctl.c   | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpi/mpi2_init.h b/drivers/scsi/mpt3sas/mpi/mpi2_init.h
index 8f1b903fe0a9..80bcf7d83184 100644
--- a/drivers/scsi/mpt3sas/mpi/mpi2_init.h
+++ b/drivers/scsi/mpt3sas/mpi/mpi2_init.h
@@ -428,7 +428,7 @@ typedef struct _MPI2_SCSI_TASK_MANAGE_REQUEST {
 	U16 Reserved3;		/*0x0A */
 	U8 LUN[8];		/*0x0C */
 	U32 Reserved4[7];	/*0x14 */
-	U16 TaskMID;		/*0x30 */
+	__le16 TaskMID;		/*0x30 */
 	U16 Reserved5;		/*0x32 */
 } MPI2_SCSI_TASK_MANAGE_REQUEST,
 	*PTR_MPI2_SCSI_TASK_MANAGE_REQUEST,
diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
index d92ca140d298..eac253fce2da 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
@@ -618,7 +618,8 @@ _ctl_set_task_mid(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command *karg,
 		 * first outstanding smid will be picked up.  Otherwise,
 		 * targeted smid will be the one.
 		 */
-		if (!tm_request->TaskMID || tm_request->TaskMID == st->smid) {
+		if (!tm_request->TaskMID ||
+		    tm_request->TaskMID == cpu_to_le16(st->smid)) {
 			tm_request->TaskMID = cpu_to_le16(st->smid);
 			found = 1;
 		}
-- 
2.35.1

[PATCH 2/5] scsi: mpt3sas: Fix writel() use

[Damien Le Moal]

writel() internally execute cpu_to_le32() to convert the vale being
wrritten to little endian. The caller should thus not use this
conversion function for the value passed to writel(). Remove the
cpu_to_le32() calls in _base_put_smid_scsi_io_atomic(),
_base_put_smid_fast_path_atomic(), _base_put_smid_hi_priority_atomic()
_base_put_smid_default_atomic() amd _base_handshake_req_reply_wait().

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 511726f92d9a..6ebdfedd84f5 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -4323,7 +4323,7 @@ _base_put_smid_scsi_io_atomic(struct MPT3SAS_ADAPTER *ioc, u16 smid,
 	descriptor.MSIxIndex = _base_set_and_get_msix_index(ioc, smid);
 	descriptor.SMID = cpu_to_le16(smid);
 
-	writel(cpu_to_le32(*request), &ioc->chip->AtomicRequestDescriptorPost);
+	writel(*request, &ioc->chip->AtomicRequestDescriptorPost);
 }
 
 /**
@@ -4345,7 +4345,7 @@ _base_put_smid_fast_path_atomic(struct MPT3SAS_ADAPTER *ioc, u16 smid,
 	descriptor.MSIxIndex = _base_set_and_get_msix_index(ioc, smid);
 	descriptor.SMID = cpu_to_le16(smid);
 
-	writel(cpu_to_le32(*request), &ioc->chip->AtomicRequestDescriptorPost);
+	writel(*request, &ioc->chip->AtomicRequestDescriptorPost);
 }
 
 /**
@@ -4368,7 +4368,7 @@ _base_put_smid_hi_priority_atomic(struct MPT3SAS_ADAPTER *ioc, u16 smid,
 	descriptor.MSIxIndex = msix_task;
 	descriptor.SMID = cpu_to_le16(smid);
 
-	writel(cpu_to_le32(*request), &ioc->chip->AtomicRequestDescriptorPost);
+	writel(*request, &ioc->chip->AtomicRequestDescriptorPost);
 }
 
 /**
@@ -4389,7 +4389,7 @@ _base_put_smid_default_atomic(struct MPT3SAS_ADAPTER *ioc, u16 smid)
 	descriptor.MSIxIndex = _base_set_and_get_msix_index(ioc, smid);
 	descriptor.SMID = cpu_to_le16(smid);
 
-	writel(cpu_to_le32(*request), &ioc->chip->AtomicRequestDescriptorPost);
+	writel(*request, &ioc->chip->AtomicRequestDescriptorPost);
 }
 
 /**
@@ -6906,7 +6906,7 @@ _base_handshake_req_reply_wait(struct MPT3SAS_ADAPTER *ioc, int request_bytes,
 
 	/* send message 32-bits at a time */
 	for (i = 0, failed = 0; i < request_bytes/4 && !failed; i++) {
-		writel(cpu_to_le32(request[i]), &ioc->chip->Doorbell);
+		writel(request[i], &ioc->chip->Doorbell);
 		if ((_base_wait_for_doorbell_ack(ioc, 5)))
 			failed = 1;
 	}
-- 
2.35.1

[PATCH 3/5] scsi: mpt3sas: fix ioc->base_readl() use

[Damien Le Moal]

The functions _base_readl_aero() and _base_readl() used for an adapter
base_readl() method are implemented using a regular readl() call which
performs internally a conversion to CPU endianness (le32_to_cpu()) of
the values read. The users of the ioc base_readl() method should thus
not convert again the values read using le16_to_cpu().
Fixing this removes sparse warnings.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 6ebdfedd84f5..5efe4bd186db 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -6925,16 +6925,16 @@ _base_handshake_req_reply_wait(struct MPT3SAS_ADAPTER *ioc, int request_bytes,
 	}
 
 	/* read the first two 16-bits, it gives the total length of the reply */
-	reply[0] = le16_to_cpu(ioc->base_readl(&ioc->chip->Doorbell)
-	    & MPI2_DOORBELL_DATA_MASK);
+	reply[0] = ioc->base_readl(&ioc->chip->Doorbell)
+		& MPI2_DOORBELL_DATA_MASK;
 	writel(0, &ioc->chip->HostInterruptStatus);
 	if ((_base_wait_for_doorbell_int(ioc, 5))) {
 		ioc_err(ioc, "doorbell handshake int failed (line=%d)\n",
 			__LINE__);
 		return -EFAULT;
 	}
-	reply[1] = le16_to_cpu(ioc->base_readl(&ioc->chip->Doorbell)
-	    & MPI2_DOORBELL_DATA_MASK);
+	reply[1] = ioc->base_readl(&ioc->chip->Doorbell)
+		& MPI2_DOORBELL_DATA_MASK;
 	writel(0, &ioc->chip->HostInterruptStatus);
 
 	for (i = 2; i < default_reply->MsgLength * 2; i++)  {
@@ -6946,9 +6946,8 @@ _base_handshake_req_reply_wait(struct MPT3SAS_ADAPTER *ioc, int request_bytes,
 		if (i >=  reply_bytes/2) /* overflow case */
 			ioc->base_readl(&ioc->chip->Doorbell);
 		else
-			reply[i] = le16_to_cpu(
-			    ioc->base_readl(&ioc->chip->Doorbell)
-			    & MPI2_DOORBELL_DATA_MASK);
+			reply[i] = ioc->base_readl(&ioc->chip->Doorbell)
+				& MPI2_DOORBELL_DATA_MASK;
 		writel(0, &ioc->chip->HostInterruptStatus);
 	}
 
-- 
2.35.1

[PATCH 4/5] scsi: mpt3sas: fix event callback log_code value handling

[Damien Le Moal]

In mpt3sas_scsih_event_callback(), fix a sparse warning when testing the
event log code value by replacing the use of a pointer to the address
storing the event log code with a log code local variable . Doing so,
le32_to_cpu() is used when the log code value is assigned, avoiding a
sparse warning.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index 00792767c620..a355bc145577 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -10926,20 +10926,20 @@ mpt3sas_scsih_event_callback(struct MPT3SAS_ADAPTER *ioc, u8 msix_index,
 	case MPI2_EVENT_LOG_ENTRY_ADDED:
 	{
 		Mpi2EventDataLogEntryAdded_t *log_entry;
-		u32 *log_code;
+		u32 log_code;
 
 		if (!ioc->is_warpdrive)
 			break;
 
 		log_entry = (Mpi2EventDataLogEntryAdded_t *)
 		    mpi_reply->EventData;
-		log_code = (u32 *)log_entry->LogData;
+		log_code = le32_to_cpu(*(__le32 *)log_entry->LogData);
 
 		if (le16_to_cpu(log_entry->LogEntryQualifier)
 		    != MPT2_WARPDRIVE_LOGENTRY)
 			break;
 
-		switch (le32_to_cpu(*log_code)) {
+		switch (log_code) {
 		case MPT2_WARPDRIVE_LC_SSDT:
 			ioc_warn(ioc, "WarpDrive Warning: IO Throttling has occurred in the WarpDrive subsystem. Check WarpDrive documentation for additional details.\n");
 			break;
-- 
2.35.1

[PATCH 5/5] scsi: mpt3sas: fix adapter replyPostRegisterIndex handling

[Damien Le Moal]

The replyPostRegisterIndex array of struct MPT3SAS_ADAPTER is regular
memory allocated from RAM, and not an IO mem resource. writel() should
thus not be used to assign values to the array entries. Replace the
writel() call modifying the array with direct assignements. This change
suppresses sparse warnings.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 37 ++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 5efe4bd186db..4caa719b4ef4 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -1771,10 +1771,13 @@ _base_process_reply_queue(struct adapter_reply_queue *reply_q)
 		 */
 		if (completed_cmds >= ioc->thresh_hold) {
 			if (ioc->combined_reply_queue) {
-				writel(reply_q->reply_post_host_index |
-						((msix_index  & 7) <<
-						 MPI2_RPHI_MSIX_INDEX_SHIFT),
-				    ioc->replyPostRegisterIndex[msix_index/8]);
+				unsigned long idx =
+					reply_q->reply_post_host_index |
+					((msix_index  & 7) <<
+					 MPI2_RPHI_MSIX_INDEX_SHIFT);
+
+				ioc->replyPostRegisterIndex[msix_index/8] =
+					(resource_size_t *) idx;
 			} else {
 				writel(reply_q->reply_post_host_index |
 						(msix_index <<
@@ -1826,14 +1829,17 @@ _base_process_reply_queue(struct adapter_reply_queue *reply_q)
 	 * new reply host index value in ReplyPostIndex Field and msix_index
 	 * value in MSIxIndex field.
 	 */
-	if (ioc->combined_reply_queue)
-		writel(reply_q->reply_post_host_index | ((msix_index  & 7) <<
-			MPI2_RPHI_MSIX_INDEX_SHIFT),
-			ioc->replyPostRegisterIndex[msix_index/8]);
-	else
+	if (ioc->combined_reply_queue) {
+		unsigned long idx = reply_q->reply_post_host_index |
+			((msix_index  & 7) << MPI2_RPHI_MSIX_INDEX_SHIFT);
+
+		ioc->replyPostRegisterIndex[msix_index/8] =
+			(resource_size_t *) idx;
+	} else {
 		writel(reply_q->reply_post_host_index | (msix_index <<
 			MPI2_RPHI_MSIX_INDEX_SHIFT),
 			&ioc->chip->ReplyPostHostIndex);
+	}
 	atomic_dec(&reply_q->busy);
 	return completed_cmds;
 }
@@ -8095,14 +8101,17 @@ _base_make_ioc_operational(struct MPT3SAS_ADAPTER *ioc)
 
 	/* initialize reply post host index */
 	list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {
-		if (ioc->combined_reply_queue)
-			writel((reply_q->msix_index & 7)<<
-			   MPI2_RPHI_MSIX_INDEX_SHIFT,
-			   ioc->replyPostRegisterIndex[reply_q->msix_index/8]);
-		else
+		if (ioc->combined_reply_queue) {
+			unsigned long idx =(reply_q->msix_index & 7) <<
+				MPI2_RPHI_MSIX_INDEX_SHIFT;
+
+			ioc->replyPostRegisterIndex[reply_q->msix_index/8] =
+				(resource_size_t *) idx;
+		} else {
 			writel(reply_q->msix_index <<
 				MPI2_RPHI_MSIX_INDEX_SHIFT,
 				&ioc->chip->ReplyPostHostIndex);
+		}
 
 		if (!_base_is_controller_msix_enabled(ioc))
 			goto skip_init_reply_post_host_index;
-- 
2.35.1

Re: [PATCH 1/5] scsi: mpt3sas: fix Mpi2SCSITaskManagementRequest_t TaskMID handling

[Bart Van Assche]

On 2/24/22 02:11, Damien Le Moal wrote:
> diff --git a/drivers/scsi/mpt3sas/mpi/mpi2_init.h b/drivers/scsi/mpt3sas/mpi/mpi2_init.h
> index 8f1b903fe0a9..80bcf7d83184 100644
> --- a/drivers/scsi/mpt3sas/mpi/mpi2_init.h
> +++ b/drivers/scsi/mpt3sas/mpi/mpi2_init.h
> @@ -428,7 +428,7 @@ typedef struct _MPI2_SCSI_TASK_MANAGE_REQUEST {
>   	U16 Reserved3;		/*0x0A */
>   	U8 LUN[8];		/*0x0C */
>   	U32 Reserved4[7];	/*0x14 */
> -	U16 TaskMID;		/*0x30 */
> +	__le16 TaskMID;		/*0x30 */
>   	U16 Reserved5;		/*0x32 */
>   } MPI2_SCSI_TASK_MANAGE_REQUEST,
>   	*PTR_MPI2_SCSI_TASK_MANAGE_REQUEST,

Is this change necessary? From drivers/scsi/mpt3sas/mpi/mpi2_type.h:

typedef __le16 U16;

BTW, I think the U16 etc. typedefs should disappear.

Thanks,

Bart.

Re: [PATCH 1/5] scsi: mpt3sas: fix Mpi2SCSITaskManagementRequest_t TaskMID handling

[Damien Le Moal]

On 2/25/22 04:49, Bart Van Assche wrote:
> On 2/24/22 02:11, Damien Le Moal wrote:
>> diff --git a/drivers/scsi/mpt3sas/mpi/mpi2_init.h b/drivers/scsi/mpt3sas/mpi/mpi2_init.h
>> index 8f1b903fe0a9..80bcf7d83184 100644
>> --- a/drivers/scsi/mpt3sas/mpi/mpi2_init.h
>> +++ b/drivers/scsi/mpt3sas/mpi/mpi2_init.h
>> @@ -428,7 +428,7 @@ typedef struct _MPI2_SCSI_TASK_MANAGE_REQUEST {
>>   	U16 Reserved3;		/*0x0A */
>>   	U8 LUN[8];		/*0x0C */
>>   	U32 Reserved4[7];	/*0x14 */
>> -	U16 TaskMID;		/*0x30 */
>> +	__le16 TaskMID;		/*0x30 */
>>   	U16 Reserved5;		/*0x32 */
>>   } MPI2_SCSI_TASK_MANAGE_REQUEST,
>>   	*PTR_MPI2_SCSI_TASK_MANAGE_REQUEST,
> 
> Is this change necessary? From drivers/scsi/mpt3sas/mpi/mpi2_type.h:
> 
> typedef __le16 U16;

Well, sparse seems to not catch that. Without the change, I get
warnings. Will check again.

> 
> BTW, I think the U16 etc. typedefs should disappear.

I agree on that :)

> 
> Thanks,
> 
> Bart.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/5] scsi: mpt3sas: fix Mpi2SCSITaskManagementRequest_t TaskMID handling

[Damien Le Moal]

On 2/25/22 04:49, Bart Van Assche wrote:
> On 2/24/22 02:11, Damien Le Moal wrote:
>> diff --git a/drivers/scsi/mpt3sas/mpi/mpi2_init.h b/drivers/scsi/mpt3sas/mpi/mpi2_init.h
>> index 8f1b903fe0a9..80bcf7d83184 100644
>> --- a/drivers/scsi/mpt3sas/mpi/mpi2_init.h
>> +++ b/drivers/scsi/mpt3sas/mpi/mpi2_init.h
>> @@ -428,7 +428,7 @@ typedef struct _MPI2_SCSI_TASK_MANAGE_REQUEST {
>>   	U16 Reserved3;		/*0x0A */
>>   	U8 LUN[8];		/*0x0C */
>>   	U32 Reserved4[7];	/*0x14 */
>> -	U16 TaskMID;		/*0x30 */
>> +	__le16 TaskMID;		/*0x30 */
>>   	U16 Reserved5;		/*0x32 */
>>   } MPI2_SCSI_TASK_MANAGE_REQUEST,
>>   	*PTR_MPI2_SCSI_TASK_MANAGE_REQUEST,
> 
> Is this change necessary? From drivers/scsi/mpt3sas/mpi/mpi2_type.h:
> 
> typedef __le16 U16;

Bart,

You are correct. The type change is not needed. Sending v2 with the update.

Thanks for checking.

> 
> BTW, I think the U16 etc. typedefs should disappear.
> 
> Thanks,
> 
> Bart.


-- 
Damien Le Moal
Western Digital Research