On 10/17/2013 05:10 PM, Saxena, Sumit wrote:
>
>> -----Original Message-----
>> From: Tomas Henzl [mailto:the...@redhat.com]
>> Sent: Thursday, October 17, 2013 7:35 PM
>> To: Saxena, Sumit; linux-scsi@vger.kernel.org
>> Cc: jbottom...@parallels.com; Desai, Kashyap; aradf...@gmail.com
>> Subject: Re: [PATCH][SCSI] megaraid_sas: Fix synchronization problem
>> between sysPD IO path and AEN path
>>
>> On 10/16/2013 01:34 PM, sumit.sax...@lsi.com 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, what remains is my second question
- 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?
>
>
> 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 <adam.radf...@lsi.com>
>>> Signed-off-by: Sumit Saxena <sumit.sax...@lsi.com>
>>> ---
>>> 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 majord...@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
