| Message ID | 20220224233056.398054-6-damien.lemoal@opensource.wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix mpt3sas driver sparse warnings | expand |
On Fri, Feb 25, 2022 at 5:01 AM Damien Le Moal <damien.lemoal@opensource.wdc.com> wrote: > > 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. writel() is a must here. replyPostRegisterIndex array is having the iommu address. and here the driver is writing the data to these iommu addresses retrieved from replyPostRegisterIndex array. Thanks, Sreekanth > > 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 >
On 3/7/22 16:35, Sreekanth Reddy wrote: > On Fri, Feb 25, 2022 at 5:01 AM Damien Le Moal > <damien.lemoal@opensource.wdc.com> wrote: >> >> 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. > > writel() is a must here. replyPostRegisterIndex array is having the > iommu address. > and here the driver is writing the data to these iommu addresses retrieved from > replyPostRegisterIndex array. So the declaration of this array of wrong. it should be an array of __iomem void * entries, not an array of resource_size_t * entries (which by the way does not make sense to me since the use of the array is clearly to store an address, not a pointer to an address). How do you prefer this fixed ? I would rather not add local casts here and fix the declaration of the replyPostRegisterIndex array. > > Thanks, > Sreekanth > >> >> 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 >>
On 3/7/22 16:35, Sreekanth Reddy wrote: > On Fri, Feb 25, 2022 at 5:01 AM Damien Le Moal > <damien.lemoal@opensource.wdc.com> wrote: >> >> 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. > > writel() is a must here. replyPostRegisterIndex array is having the > iommu address. > and here the driver is writing the data to these iommu addresses retrieved from > replyPostRegisterIndex array. Fixed in v3. Please check.
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;
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(-)