>-----Original Message----- >From: Tomas Henzl [mailto:[email protected]] >Sent: Thursday, October 17, 2013 7:35 PM >To: Saxena, Sumit; [email protected] >Cc: [email protected]; Desai, Kashyap; [email protected] >Subject: Re: [PATCH][SCSI] megaraid_sas: Fix synchronization problem >between sysPD IO path and AEN path > >On 10/16/2013 01:34 PM, [email protected] wrote: >> There is syncronization problem between sysPD IO path and AEN path. >Driver maintains instance->pd_list[] array, which will get updated(by >calling function megasas_get_pd_list[]), whenever any of below events >occurs- > >Hi Sumit, >- I'm a bit confused here- there are two threads which might access the >same array, but the problem is still there > when the second thread accesses the array during the final memcpy, I >have expected that you will add some locking, > but maybe I'm missing something. >- now the code zeroes the pd_list even when the > (ret == 0 && (ci->count < (MEGASAS_MAX_PD_CHANNELS * >MEGASAS_MAX_DEV_PER_CHANNEL))) > is not true. This is I think new - is that intentional?
Tomas, Having lock to synchronize this will be a good choice, but will need changes in multiple places. Without this patch: driver memsets instance->pd_list[] array to zero, same array will be accessed in sysPD IO path, that creates problem. To resolve this issue, we introduced a new array "instance->local_pd_list[]" array, which we will be filling from Firmware DMAed data and then finally memcpy that array to the "instance->pd_list[]". Since "instance->pd_list" is accessed in IO path, then no problem in memset zero here(memset is on "instance->local_pd_list"). Final "Memcpy" operation is not saved with locking, reason is: "instance->pd_list" array is of type "struct megasas_pd_list", which is of 32-bit, so single entry in "instance->local_pd_list" array will be copied in one CPU cycle, and with current MR FW design, it will not be a problem even if IO path (or any other thread) is accessing old instance->pd_list[]. so we are safe here in memcpy() here. Adding lock will add overhead in IO path, which could be avoided is main reason to resolve this issue with this fix. Thanks, Sumit >Thanks, Tomas > > >> >> >> >> >> >> >> MR_EVT_PD_INSERTED >> MR_EVT_PD_REMOVED >> MR_EVT_CTRL_HOST_BUS_SCAN_REQUESTED >> MR_EVT_FOREIGN_CFG_IMPORTED >> >> At same time running sysPD IO will be accessing the same array >instance->pd_list[], which is getting updated in AEN path, because >> of this IO may not get correct PD info from instance->pd_list[] array. >> >> Signed-off-by: Adam Radford <[email protected]> >> Signed-off-by: Sumit Saxena <[email protected]> >> --- >> diff --git a/drivers/scsi/megaraid/megaraid_sas.h >b/drivers/scsi/megaraid/megaraid_sas.h >> index 0c73ba4..e9e543c 100644 >> --- a/drivers/scsi/megaraid/megaraid_sas.h >> +++ b/drivers/scsi/megaraid/megaraid_sas.h >> @@ -1531,6 +1531,7 @@ struct megasas_instance { >> struct megasas_register_set __iomem *reg_set; >> u32 *reply_post_host_index_addr[MR_MAX_MSIX_REG_ARRAY]; >> struct megasas_pd_list pd_list[MEGASAS_MAX_PD]; >> + struct megasas_pd_list local_pd_list[MEGASAS_MAX_PD]; >> u8 ld_ids[MEGASAS_MAX_LD_IDS]; >> s8 init_id; >> >> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c >b/drivers/scsi/megaraid/megaraid_sas_base.c >> index e62ff02..83ebc75 100644 >> --- a/drivers/scsi/megaraid/megaraid_sas_base.c >> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c >> @@ -3194,21 +3194,23 @@ megasas_get_pd_list(struct megasas_instance >*instance) >> (le32_to_cpu(ci->count) < >> (MEGASAS_MAX_PD_CHANNELS * MEGASAS_MAX_DEV_PER_CHANNEL))) >{ >> >> - memset(instance->pd_list, 0, >> + memset(instance->local_pd_list, 0, >> MEGASAS_MAX_PD * sizeof(struct megasas_pd_list)); >> >> for (pd_index = 0; pd_index < le32_to_cpu(ci->count); >pd_index++) { >> >> - instance->pd_list[le16_to_cpu(pd_addr->deviceId)].tid > = >> + instance->local_pd_list[le16_to_cpu(pd_addr- >>deviceId)].tid = >> le16_to_cpu(pd_addr->deviceId); >> - instance->pd_list[le16_to_cpu(pd_addr- >>deviceId)].driveType = >> + instance->local_pd_list[le16_to_cpu(pd_addr- >>deviceId)].driveType = >> pd_addr->scsiDevType; >> - instance->pd_list[le16_to_cpu(pd_addr- >>deviceId)].driveState = >> + instance->local_pd_list[le16_to_cpu(pd_addr- >>deviceId)].driveState = >> MR_PD_STATE_SYSTEM; >> pd_addr++; >> } >> } >> >> + memcpy(instance->pd_list, instance->local_pd_list, >> + sizeof(instance->pd_list)); >> pci_free_consistent(instance->pdev, >> MEGASAS_MAX_PD * sizeof(struct MR_PD_LIST), >> ci, ci_h); >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-scsi" >in >> the body of a message to [email protected] >> More majordomo info at http://vger.kernel.org/majordomo-info.html >
