Re: [PATCH 03/15] be2iscsi: Fix to use atomic operations for tag_state
On 12/20/2015 01:44 AM, Mike Christie wrote:
>> diff --git a/drivers/scsi/be2iscsi/be_cmds.c
>> b/drivers/scsi/be2iscsi/be_cmds.c
>> index 6fabded..21c806f 100644
>> --- a/drivers/scsi/be2iscsi/be_cmds.c
>> +++ b/drivers/scsi/be2iscsi/be_cmds.c
>> @@ -164,9 +164,8 @@ int beiscsi_mccq_compl(struct beiscsi_hba *phba,
>> }
>>
>> /* Set MBX Tag state to Active */
>> -mutex_lock(&phba->ctrl.mbox_lock);
>> -phba->ctrl.ptag_state[tag].tag_state = MCC_TAG_STATE_RUNNING;
>> -mutex_unlock(&phba->ctrl.mbox_lock);
>> +atomic_set(&phba->ctrl.ptag_state[tag].tag_state,
>> +MCC_TAG_STATE_RUNNING);
>>
Is it possible for be_mcc_compl_process_isr to run before we have set
the state to MCC_TAG_STATE_RUNNING, so the
wait_event_interruptible_timeout call timesout?
>> /* wait for the mccq completion */
>> rc = wait_event_interruptible_timeout(
>> @@ -178,9 +177,8 @@ int beiscsi_mccq_compl(struct beiscsi_hba *phba,
>> if (rc <= 0) {
>> struct be_dma_mem *tag_mem;
>> /* Set MBX Tag state to timeout */
>> -mutex_lock(&phba->ctrl.mbox_lock);
>> -phba->ctrl.ptag_state[tag].tag_state = MCC_TAG_STATE_TIMEOUT;
>> -mutex_unlock(&phba->ctrl.mbox_lock);
>> +atomic_set(&phba->ctrl.ptag_state[tag].tag_state,
>> +MCC_TAG_STATE_TIMEOUT);
>>
>> /* Store resource addr to be freed later */
>> tag_mem = &phba->ctrl.ptag_state[tag].tag_mem_state;
>> @@ -199,9 +197,8 @@ int beiscsi_mccq_compl(struct beiscsi_hba *phba,
>> } else {
>> rc = 0;
>> /* Set MBX Tag state to completed */
>> -mutex_lock(&phba->ctrl.mbox_lock);
>> -phba->ctrl.ptag_state[tag].tag_state = MCC_TAG_STATE_COMPLETED;
>> -mutex_unlock(&phba->ctrl.mbox_lock);
>> +atomic_set(&phba->ctrl.ptag_state[tag].tag_state,
>> +MCC_TAG_STATE_COMPLETED);
>> }
>>
>> mcc_tag_response = phba->ctrl.mcc_numtag[tag];
>> @@ -373,9 +370,11 @@ int be_mcc_compl_process_isr(struct be_ctrl_info *ctrl,
>> ctrl->mcc_numtag[tag] |= (extd_status & 0x00FF) << 8;
>> ctrl->mcc_numtag[tag] |= (compl_status & 0x00FF);
>>
>> -if (ctrl->ptag_state[tag].tag_state == MCC_TAG_STATE_RUNNING) {
>> +if (atomic_read(&ctrl->ptag_state[tag].tag_state) ==
>> +MCC_TAG_STATE_RUNNING) {
>> wake_up_interruptible(&ctrl->mcc_wait[tag]);
>> -} else if (ctrl->ptag_state[tag].tag_state == MCC_TAG_STATE_TIMEOUT) {
>> +} else if (atomic_read(&ctrl->ptag_state[tag].tag_state) ==
>> +MCC_TAG_STATE_TIMEOUT) {
>> struct be_dma_mem *tag_mem;
>> tag_mem = &ctrl->ptag_state[tag].tag_mem_state;
>>
>> @@ -390,9 +389,8 @@ int be_mcc_compl_process_isr(struct be_ctrl_info *ctrl,
>> tag_mem->va, tag_mem->dma);
>>
>> /* Change tag state */
>> -mutex_lock(&phba->ctrl.mbox_lock);
>> -ctrl->ptag_state[tag].tag_state = MCC_TAG_STATE_COMPLETED;
>> -mutex_unlock(&phba->ctrl.mbox_lock);
>> +atomic_set(&ctrl->ptag_state[tag].tag_state,
>> +MCC_TAG_STATE_COMPLETED);
>>
>> /* Free MCC Tag */
>> free_mcc_tag(ctrl, tag);
>>
>
> I think if you only need to get and set a u8 then you can just use a u8
> since the operation will be atomic. No need for a atomic_t.
I think you can ignore this. I think you would need some barriers in
there too and it might be more complicated for no gain.
--
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
Re: [PATCH 03/15] be2iscsi: Fix to use atomic operations for tag_state
On 12/20/15 3:01 AM, Mike Christie wrote:
On 12/20/2015 01:44 AM, Mike Christie wrote:
diff --git a/drivers/scsi/be2iscsi/be_cmds.c b/drivers/scsi/be2iscsi/be_cmds.c
index 6fabded..21c806f 100644
--- a/drivers/scsi/be2iscsi/be_cmds.c
+++ b/drivers/scsi/be2iscsi/be_cmds.c
@@ -164,9 +164,8 @@ int beiscsi_mccq_compl(struct beiscsi_hba *phba,
}
/* Set MBX Tag state to Active */
- mutex_lock(&phba->ctrl.mbox_lock);
- phba->ctrl.ptag_state[tag].tag_state = MCC_TAG_STATE_RUNNING;
- mutex_unlock(&phba->ctrl.mbox_lock);
+ atomic_set(&phba->ctrl.ptag_state[tag].tag_state,
+ MCC_TAG_STATE_RUNNING);
Is it possible for be_mcc_compl_process_isr to run before we have set
the state to MCC_TAG_STATE_RUNNING, so the
wait_event_interruptible_timeout call timesout?
/* wait for the mccq completion */
rc = wait_event_interruptible_timeout(
@@ -178,9 +177,8 @@ int beiscsi_mccq_compl(struct beiscsi_hba *phba,
if (rc <= 0) {
struct be_dma_mem *tag_mem;
/* Set MBX Tag state to timeout */
- mutex_lock(&phba->ctrl.mbox_lock);
- phba->ctrl.ptag_state[tag].tag_state = MCC_TAG_STATE_TIMEOUT;
- mutex_unlock(&phba->ctrl.mbox_lock);
+ atomic_set(&phba->ctrl.ptag_state[tag].tag_state,
+ MCC_TAG_STATE_TIMEOUT);
/* Store resource addr to be freed later */
tag_mem = &phba->ctrl.ptag_state[tag].tag_mem_state;
@@ -199,9 +197,8 @@ int beiscsi_mccq_compl(struct beiscsi_hba *phba,
} else {
rc = 0;
/* Set MBX Tag state to completed */
- mutex_lock(&phba->ctrl.mbox_lock);
- phba->ctrl.ptag_state[tag].tag_state = MCC_TAG_STATE_COMPLETED;
- mutex_unlock(&phba->ctrl.mbox_lock);
+ atomic_set(&phba->ctrl.ptag_state[tag].tag_state,
+ MCC_TAG_STATE_COMPLETED);
}
mcc_tag_response = phba->ctrl.mcc_numtag[tag];
@@ -373,9 +370,11 @@ int be_mcc_compl_process_isr(struct be_ctrl_info *ctrl,
ctrl->mcc_numtag[tag] |= (extd_status & 0x00FF) << 8;
ctrl->mcc_numtag[tag] |= (compl_status & 0x00FF);
- if (ctrl->ptag_state[tag].tag_state == MCC_TAG_STATE_RUNNING) {
+ if (atomic_read(&ctrl->ptag_state[tag].tag_state) ==
+ MCC_TAG_STATE_RUNNING) {
wake_up_interruptible(&ctrl->mcc_wait[tag]);
- } else if (ctrl->ptag_state[tag].tag_state == MCC_TAG_STATE_TIMEOUT) {
+ } else if (atomic_read(&ctrl->ptag_state[tag].tag_state) ==
+ MCC_TAG_STATE_TIMEOUT) {
struct be_dma_mem *tag_mem;
tag_mem = &ctrl->ptag_state[tag].tag_mem_state;
@@ -390,9 +389,8 @@ int be_mcc_compl_process_isr(struct be_ctrl_info *ctrl,
tag_mem->va, tag_mem->dma);
/* Change tag state */
- mutex_lock(&phba->ctrl.mbox_lock);
- ctrl->ptag_state[tag].tag_state = MCC_TAG_STATE_COMPLETED;
- mutex_unlock(&phba->ctrl.mbox_lock);
+ atomic_set(&ctrl->ptag_state[tag].tag_state,
+ MCC_TAG_STATE_COMPLETED);
/* Free MCC Tag */
free_mcc_tag(ctrl, tag);
I think if you only need to get and set a u8 then you can just use a u8
since the operation will be atomic. No need for a atomic_t.
I think you can ignore this. I think you would need some barriers in
there too and it might be more complicated for no gain.
Ughhh. Sorry.
The atomic_ops.txt doc says atomic_read/atomic_set use still needs
barriers, so I guess they do not do anything for you and a u8 is ok.
The memory-barrier.txt doc says wake_up/wait calls have barriers if the
wake_up path is hit, so you are ok there.
However, besides the timeout issue in the previous mail, can
beiscsi_mccq_compl set the tag_mem_state to MCC_TAG_STATE_TIMEOUT and
be_mcc_compl_process_isr does not see the tag_mem values updated?
--
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
RE: [PATCH 10/15] be2iscsi: Add FW config validation
It stands for Dual ULP Aware. Will add a comment explaining that.
Thanks,
JB
-Original Message-
From: Hannes Reinecke [mailto:h...@suse.de]
Sent: Friday, December 18, 2015 2:34 PM
To: Jitendra Bhivare; linux-scsi@vger.kernel.org; micha...@cs.wisc.edu
Subject: Re: [PATCH 10/15] be2iscsi: Add FW config validation
On 12/15/2015 04:55 PM, Jitendra Bhivare wrote:
> From: Jitendra
>
> System crash in I+T card personality
>
> Fix to add validation for ULP in initiator mode, physical port number,
> and supported queue, icd, cid counts.
>
> Signed-off-by: Jitendra
> ---
> drivers/scsi/be2iscsi/be_main.c |2 +-
> drivers/scsi/be2iscsi/be_main.h |2 +
> drivers/scsi/be2iscsi/be_mgmt.c | 188
+--
> 3 files changed, 123 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/scsi/be2iscsi/be_main.c
> b/drivers/scsi/be2iscsi/be_main.c index 8967e05..a665e6a 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -5670,6 +5670,7 @@ static int beiscsi_dev_probe(struct pci_dev
*pcidev,
> goto free_port;
> }
> mgmt_get_port_name(&phba->ctrl, phba);
> + beiscsi_get_params(phba);
>
> if (enable_msix)
> find_num_cpus(phba);
> @@ -5687,7 +5688,6 @@ static int beiscsi_dev_probe(struct pci_dev
*pcidev,
> }
>
> phba->shost->max_id = phba->params.cxns_per_ctrl;
> - beiscsi_get_params(phba);
> phba->shost->can_queue = phba->params.ios_per_ctrl;
> ret = beiscsi_init_port(phba);
> if (ret < 0) {
> diff --git a/drivers/scsi/be2iscsi/be_main.h
> b/drivers/scsi/be2iscsi/be_main.h index c09082a..f89861b 100644
> --- a/drivers/scsi/be2iscsi/be_main.h
> +++ b/drivers/scsi/be2iscsi/be_main.h
> @@ -397,7 +397,9 @@ struct beiscsi_hba {
>* group together since they are used most frequently
>* for cid to cri conversion
>*/
> +#define BEISCSI_PHYS_PORT_MAX4
> unsigned int phys_port;
> + /* valid values of phys_port id are 0, 1, 2, 3 */
> unsigned int eqid_count;
> unsigned int cqid_count;
> unsigned int iscsi_cid_start[BEISCSI_ULP_COUNT];
> diff --git a/drivers/scsi/be2iscsi/be_mgmt.c
> b/drivers/scsi/be2iscsi/be_mgmt.c index ad7aa75..15f7ad7 100644
> --- a/drivers/scsi/be2iscsi/be_mgmt.c
> +++ b/drivers/scsi/be2iscsi/be_mgmt.c
> @@ -373,90 +373,142 @@ int mgmt_get_fw_config(struct be_ctrl_info *ctrl,
> struct beiscsi_hba *phba)
> {
> struct be_mcc_wrb *wrb = wrb_from_mbox(&ctrl->mbox_mem);
> - struct be_fw_cfg *req = embedded_payload(wrb);
> - int status = 0;
> + struct be_fw_cfg *pfw_cfg = embedded_payload(wrb);
> + uint32_t cid_count, icd_count;
> + int status = -EINVAL;
> + uint8_t ulp_num = 0;
>
> mutex_lock(&ctrl->mbox_lock);
> memset(wrb, 0, sizeof(*wrb));
> + be_wrb_hdr_prepare(wrb, sizeof(*pfw_cfg), true, 0);
>
> - be_wrb_hdr_prepare(wrb, sizeof(*req), true, 0);
> -
> - be_cmd_hdr_prepare(&req->hdr, CMD_SUBSYSTEM_COMMON,
> + be_cmd_hdr_prepare(&pfw_cfg->hdr, CMD_SUBSYSTEM_COMMON,
> OPCODE_COMMON_QUERY_FIRMWARE_CONFIG,
> EMBED_MBX_MAX_PAYLOAD_SIZE);
> - status = be_mbox_notify(ctrl);
> - if (!status) {
> - uint8_t ulp_num = 0;
> - struct be_fw_cfg *pfw_cfg;
> - pfw_cfg = req;
>
> - if (!is_chip_be2_be3r(phba)) {
> - phba->fw_config.eqid_count = pfw_cfg->eqid_count;
> - phba->fw_config.cqid_count = pfw_cfg->cqid_count;
> + if (be_mbox_notify(ctrl)) {
> + beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
> + "BG_%d : Failed in mgmt_get_fw_config\n");
> + goto fail_init;
> + }
>
> - beiscsi_log(phba, KERN_INFO,
> - BEISCSI_LOG_INIT,
> - "BG_%d : EQ_Count : %d CQ_Count :
%d\n",
> - phba->fw_config.eqid_count,
> + /* FW response formats depend on port id */
> + phba->fw_config.phys_port = pfw_cfg->phys_port;
> + if (phba->fw_config.phys_port >= BEISCSI_PHYS_PORT_MAX) {
> + beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
> + "BG_%d : invalid physical port id %d\n",
> + phba->fw_config.phys_port);
> + goto fail_init;
> + }
> +
> + /* populate and check FW config against min and max values */
> + if (!is_chip_be2_be3r(phba)) {
> + phba->fw_config.eqid_count = pfw_cfg->eqid_count;
> + phba->fw_config.cqid_count = pfw_cfg->cqid_count;
> + if (phba->fw_config.eqid_count == 0 ||
> + phba->fw_config.eqid_count > 2048) {
> + beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
> +
RE: [PATCH 10/15] be2iscsi: Add FW config validation
Yes, the faults are not being used, it just adds to the confusion, we just
intend to get the link state change - UP or DOWN.
-Original Message-
From: Hannes Reinecke [mailto:h...@suse.de]
Sent: Friday, December 18, 2015 2:34 PM
To: Jitendra Bhivare; linux-scsi@vger.kernel.org; micha...@cs.wisc.edu
Subject: Re: [PATCH 10/15] be2iscsi: Add FW config validation
On 12/15/2015 04:55 PM, Jitendra Bhivare wrote:
> From: Jitendra
>
> System crash in I+T card personality
>
> Fix to add validation for ULP in initiator mode, physical port number,
> and supported queue, icd, cid counts.
>
> Signed-off-by: Jitendra
> ---
> drivers/scsi/be2iscsi/be_main.c |2 +-
> drivers/scsi/be2iscsi/be_main.h |2 +
> drivers/scsi/be2iscsi/be_mgmt.c | 188
+--
> 3 files changed, 123 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/scsi/be2iscsi/be_main.c
> b/drivers/scsi/be2iscsi/be_main.c index 8967e05..a665e6a 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -5670,6 +5670,7 @@ static int beiscsi_dev_probe(struct pci_dev
*pcidev,
> goto free_port;
> }
> mgmt_get_port_name(&phba->ctrl, phba);
> + beiscsi_get_params(phba);
>
> if (enable_msix)
> find_num_cpus(phba);
> @@ -5687,7 +5688,6 @@ static int beiscsi_dev_probe(struct pci_dev
*pcidev,
> }
>
> phba->shost->max_id = phba->params.cxns_per_ctrl;
> - beiscsi_get_params(phba);
> phba->shost->can_queue = phba->params.ios_per_ctrl;
> ret = beiscsi_init_port(phba);
> if (ret < 0) {
> diff --git a/drivers/scsi/be2iscsi/be_main.h
> b/drivers/scsi/be2iscsi/be_main.h index c09082a..f89861b 100644
> --- a/drivers/scsi/be2iscsi/be_main.h
> +++ b/drivers/scsi/be2iscsi/be_main.h
> @@ -397,7 +397,9 @@ struct beiscsi_hba {
>* group together since they are used most frequently
>* for cid to cri conversion
>*/
> +#define BEISCSI_PHYS_PORT_MAX4
> unsigned int phys_port;
> + /* valid values of phys_port id are 0, 1, 2, 3 */
> unsigned int eqid_count;
> unsigned int cqid_count;
> unsigned int iscsi_cid_start[BEISCSI_ULP_COUNT];
> diff --git a/drivers/scsi/be2iscsi/be_mgmt.c
> b/drivers/scsi/be2iscsi/be_mgmt.c index ad7aa75..15f7ad7 100644
> --- a/drivers/scsi/be2iscsi/be_mgmt.c
> +++ b/drivers/scsi/be2iscsi/be_mgmt.c
> @@ -373,90 +373,142 @@ int mgmt_get_fw_config(struct be_ctrl_info *ctrl,
> struct beiscsi_hba *phba)
> {
> struct be_mcc_wrb *wrb = wrb_from_mbox(&ctrl->mbox_mem);
> - struct be_fw_cfg *req = embedded_payload(wrb);
> - int status = 0;
> + struct be_fw_cfg *pfw_cfg = embedded_payload(wrb);
> + uint32_t cid_count, icd_count;
> + int status = -EINVAL;
> + uint8_t ulp_num = 0;
>
> mutex_lock(&ctrl->mbox_lock);
> memset(wrb, 0, sizeof(*wrb));
> + be_wrb_hdr_prepare(wrb, sizeof(*pfw_cfg), true, 0);
>
> - be_wrb_hdr_prepare(wrb, sizeof(*req), true, 0);
> -
> - be_cmd_hdr_prepare(&req->hdr, CMD_SUBSYSTEM_COMMON,
> + be_cmd_hdr_prepare(&pfw_cfg->hdr, CMD_SUBSYSTEM_COMMON,
> OPCODE_COMMON_QUERY_FIRMWARE_CONFIG,
> EMBED_MBX_MAX_PAYLOAD_SIZE);
> - status = be_mbox_notify(ctrl);
> - if (!status) {
> - uint8_t ulp_num = 0;
> - struct be_fw_cfg *pfw_cfg;
> - pfw_cfg = req;
>
> - if (!is_chip_be2_be3r(phba)) {
> - phba->fw_config.eqid_count = pfw_cfg->eqid_count;
> - phba->fw_config.cqid_count = pfw_cfg->cqid_count;
> + if (be_mbox_notify(ctrl)) {
> + beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
> + "BG_%d : Failed in mgmt_get_fw_config\n");
> + goto fail_init;
> + }
>
> - beiscsi_log(phba, KERN_INFO,
> - BEISCSI_LOG_INIT,
> - "BG_%d : EQ_Count : %d CQ_Count :
%d\n",
> - phba->fw_config.eqid_count,
> + /* FW response formats depend on port id */
> + phba->fw_config.phys_port = pfw_cfg->phys_port;
> + if (phba->fw_config.phys_port >= BEISCSI_PHYS_PORT_MAX) {
> + beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
> + "BG_%d : invalid physical port id %d\n",
> + phba->fw_config.phys_port);
> + goto fail_init;
> + }
> +
> + /* populate and check FW config against min and max values */
> + if (!is_chip_be2_be3r(phba)) {
> + phba->fw_config.eqid_count = pfw_cfg->eqid_count;
> + phba->fw_config.cqid_count = pfw_cfg->cqid_count;
> + if (phba->fw_config.eqid_count == 0 ||
> + phba->fw_config.eqid_count > 2048) {
> + beiscsi_log(phba, KER
RE: [PATCH 03/15] be2iscsi: Fix to use atomic operations for tag_state
Yep, there is a need for barrier when setting MCC_TAG_STATE_TIMEOUT.
I am sorry, was under the assumption that lock prefix would be used for
that atomic operation as well.
Thanks,
JB
-Original Message-
From: Mike Christie [mailto:micha...@cs.wisc.edu]
Sent: Sunday, December 20, 2015 3:44 PM
To: Jitendra Bhivare; linux-scsi@vger.kernel.org
Subject: Re: [PATCH 03/15] be2iscsi: Fix to use atomic operations for
tag_state
On 12/20/15 3:01 AM, Mike Christie wrote:
> On 12/20/2015 01:44 AM, Mike Christie wrote:
>
>>> diff --git a/drivers/scsi/be2iscsi/be_cmds.c
>>> b/drivers/scsi/be2iscsi/be_cmds.c index 6fabded..21c806f 100644
>>> --- a/drivers/scsi/be2iscsi/be_cmds.c
>>> +++ b/drivers/scsi/be2iscsi/be_cmds.c
>>> @@ -164,9 +164,8 @@ int beiscsi_mccq_compl(struct beiscsi_hba *phba,
>>> }
>>>
>>> /* Set MBX Tag state to Active */
>>> - mutex_lock(&phba->ctrl.mbox_lock);
>>> - phba->ctrl.ptag_state[tag].tag_state = MCC_TAG_STATE_RUNNING;
>>> - mutex_unlock(&phba->ctrl.mbox_lock);
>>> + atomic_set(&phba->ctrl.ptag_state[tag].tag_state,
>>> + MCC_TAG_STATE_RUNNING);
>>>
>
>
> Is it possible for be_mcc_compl_process_isr to run before we have set
> the state to MCC_TAG_STATE_RUNNING, so the
> wait_event_interruptible_timeout call timesout?
>
>
>>> /* wait for the mccq completion */
>>> rc = wait_event_interruptible_timeout( @@ -178,9 +177,8 @@ int
>>> beiscsi_mccq_compl(struct beiscsi_hba *phba,
>>> if (rc <= 0) {
>>> struct be_dma_mem *tag_mem;
>>> /* Set MBX Tag state to timeout */
>>> - mutex_lock(&phba->ctrl.mbox_lock);
>>> - phba->ctrl.ptag_state[tag].tag_state =
MCC_TAG_STATE_TIMEOUT;
>>> - mutex_unlock(&phba->ctrl.mbox_lock);
>>> + atomic_set(&phba->ctrl.ptag_state[tag].tag_state,
>>> + MCC_TAG_STATE_TIMEOUT);
>>>
>>> /* Store resource addr to be freed later */
>>> tag_mem = &phba->ctrl.ptag_state[tag].tag_mem_state;
>>> @@ -199,9 +197,8 @@ int beiscsi_mccq_compl(struct beiscsi_hba *phba,
>>> } else {
>>> rc = 0;
>>> /* Set MBX Tag state to completed */
>>> - mutex_lock(&phba->ctrl.mbox_lock);
>>> - phba->ctrl.ptag_state[tag].tag_state =
MCC_TAG_STATE_COMPLETED;
>>> - mutex_unlock(&phba->ctrl.mbox_lock);
>>> + atomic_set(&phba->ctrl.ptag_state[tag].tag_state,
>>> + MCC_TAG_STATE_COMPLETED);
>>> }
>>>
>>> mcc_tag_response = phba->ctrl.mcc_numtag[tag]; @@ -373,9 +370,11
>>> @@ int be_mcc_compl_process_isr(struct be_ctrl_info *ctrl,
>>> ctrl->mcc_numtag[tag] |= (extd_status & 0x00FF) << 8;
>>> ctrl->mcc_numtag[tag] |= (compl_status & 0x00FF);
>>>
>>> - if (ctrl->ptag_state[tag].tag_state == MCC_TAG_STATE_RUNNING) {
>>> + if (atomic_read(&ctrl->ptag_state[tag].tag_state) ==
>>> + MCC_TAG_STATE_RUNNING) {
>>> wake_up_interruptible(&ctrl->mcc_wait[tag]);
>>> - } else if (ctrl->ptag_state[tag].tag_state ==
MCC_TAG_STATE_TIMEOUT) {
>>> + } else if (atomic_read(&ctrl->ptag_state[tag].tag_state) ==
>>> + MCC_TAG_STATE_TIMEOUT) {
>>> struct be_dma_mem *tag_mem;
>>> tag_mem = &ctrl->ptag_state[tag].tag_mem_state;
>>>
>>> @@ -390,9 +389,8 @@ int be_mcc_compl_process_isr(struct be_ctrl_info
*ctrl,
>>> tag_mem->va, tag_mem->dma);
>>>
>>> /* Change tag state */
>>> - mutex_lock(&phba->ctrl.mbox_lock);
>>> - ctrl->ptag_state[tag].tag_state = MCC_TAG_STATE_COMPLETED;
>>> - mutex_unlock(&phba->ctrl.mbox_lock);
>>> + atomic_set(&ctrl->ptag_state[tag].tag_state,
>>> + MCC_TAG_STATE_COMPLETED);
>>>
>>> /* Free MCC Tag */
>>> free_mcc_tag(ctrl, tag);
>>>
>>
>> I think if you only need to get and set a u8 then you can just use a
>> u8 since the operation will be atomic. No need for a atomic_t.
>
> I think you can ignore this. I think you would need some barriers in
> there too and it might be more complicated for no gain.
>
Ughhh. Sorry.
The atomic_ops.txt doc says atomic_read/atomic_set use still needs
barriers, so I guess they do not do anything for you and a u8 is ok.
The memory-barrier.txt doc says wake_up/wait calls have barriers if the
wake_up path is hit, so you are ok there.
However, besides the timeout issue in the previous mail, can
beiscsi_mccq_compl set the tag_mem_state to MCC_TAG_STATE_TIMEOUT and
be_mcc_compl_process_isr does not see the tag_mem values updated?
--
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
Re: [PATCH v2 00/16] qla2xxx: Patches for target-pending branch
Hi Himanshu & Co, On Thu, 2015-12-17 at 14:56 -0500, Himanshu Madhani wrote: > Hi Nic, > > Please apply this series to target-pending at your earliest convenience. > > changes from v1 -> v2 > > o Added Reviewed-by tag for reviewed patches. > > o Dropped following patches for rework. > - qla2xxx: Change check_stop_free to always return 1 > - qla2xxx: Fix interaction issue between qla2xxx and Target Core Module > - qla2xxx: Add TAS detection for kernel 3.15 n newer > - target/tmr: LUN reset cause cmd premature free. > Thanks for dropping these for the moment. I'll be following up on outstanding TMR LUN_RESET reference counting bugs over the holiday. > o Fixed patch description on following patches > - qla2xxx: Enable Extended Login support > - qla2xxx: Enable Exchange offload support. > > o Fixed patch description as well as kbuild warning for > - qla2xxx: Added interface to send explicit LOGO. > > o Fixed kbuild warning for > - qla2xxx: Remove dependency on hardware_lock to reduce lock > contention. > > Thanks, > Himanshu > > Alexei Potashnik (2): > qla2xxx: Delete session if initiator is gone from FW > qla2xxx: Wait for all conflicts before ack'ing PLOGI > > Dilip Kumar Uppugandla (1): > qla2xxx: Check for online flag instead of active reset when > transmitting responses > > Himanshu Madhani (4): > qla2xxx: Enable Extended Logins support > qla2xxx: Enable Exchange offload support. > qla2xxx: Enable Target counters in DebugFS. > qla2xxx: Added interface to send explicit LOGO. > > Quinn Tran (9): > qla2xxx: Add FW resource count in DebugFS. > qla2xxx: Replace QLA_TGT_STATE_ABORTED with a bit. > qla2xxx: Remove dependency on hardware_lock to reduce lock > contention. > qla2xxx: Add irq affinity notification > qla2xxx: Add selective command queuing > qla2xxx: Move atioq to a different lock to reduce lock contention > qla2xxx: Disable ZIO at start time. > qla2xxx: Set all queues to 4k > qla2xxx: Add bulk send for atio & ctio completion paths. > > drivers/scsi/qla2xxx/qla_attr.c| 36 ++ > drivers/scsi/qla2xxx/qla_dbg.c | 19 +- > drivers/scsi/qla2xxx/qla_def.h | 86 - > drivers/scsi/qla2xxx/qla_dfs.c | 106 + > drivers/scsi/qla2xxx/qla_gbl.h | 18 +- > drivers/scsi/qla2xxx/qla_init.c| 58 ++- > drivers/scsi/qla2xxx/qla_inline.h |2 + > drivers/scsi/qla2xxx/qla_iocb.c| 188 + > drivers/scsi/qla2xxx/qla_isr.c | 129 ++- > drivers/scsi/qla2xxx/qla_mbx.c | 265 - > drivers/scsi/qla2xxx/qla_os.c | 165 - > drivers/scsi/qla2xxx/qla_target.c | 690 > +--- > drivers/scsi/qla2xxx/qla_target.h | 36 ++- > drivers/scsi/qla2xxx/tcm_qla2xxx.c | 107 - > drivers/target/target_core_transport.c |5 +- > include/target/target_core_base.h |1 + > 16 files changed, 1671 insertions(+), 240 deletions(-) > Applied patches #1 -> #14 + #16 to target-pending/for-next code. Adding commenting inline for patch #15. Thank you, --nab -- 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
Re: [PATCH 13/15] be2iscsi: Fix to process 25G link speed info from FW
On 12/21/2015 04:00 AM, Jitendra Bhivare wrote: Yes, the faults are not being used, it just adds to the confusion, where we just want to get the link state change up or down. So make it a separate patch explaining this. Cheers, Hannes -- Dr. Hannes ReineckezSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) -- 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
Re: [PATCH v2 15/16] qla2xxx: Add bulk send for atio & ctio completion paths.
On Thu, 2015-12-17 at 14:57 -0500, Himanshu Madhani wrote: > From: Quinn Tran > > At high traffic, the work queue can become a bottle neck. > Instead of putting each command on the work queue as 1 work > element, the fix would daisy chain a list of commands that came > from FW/interrupt under 1 work element to reduce lock contention. > I'm wondering if we are better served by turning this into generic logic in kernel/workqueue.c to be used beyond qla2xxx, or: using WQ_UNBOUND (following iser-target) and verify if observed bottleneck is due to internal !(WQ_UNBOUND) usage in qla_target.c. Out of curiosity, what level of performance improvement does this patch (as is) actually provide..? Thank you, --nab -- 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
Re: [PATCH 0/2] target: kthread login failure hung task + CAW use-after-free
On Fri, 2015-12-18 at 14:05 +0100, Martin Svec wrote: > Hello Nick, > > should these patches be included in 4.1.x LTS series too?: > > Nicholas Bellinger (2): > iscsi-target: Fix rx_login_comp hang after login failure > target: Fix race for SCF_COMPARE_AND_WRITE_POST checking > > They are marked as #v3.10+ and #v3.12+ but I still don't see them in 4.1.15. > FYI, Kamal has applied both to v3.19.y + v3.13.y stable code. Me thinks Greg-KH stable backlog is higher than usual due to holidays, etc. I'm sure he will be getting back to it soon. ;) --nab -- 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
Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
On 12/19/2015 03:28 AM, KY Srinivasan wrote: [ .. ] Could you? You're making what you describe as an optimisation but there are two reasons why this might not be so. The first is that the compiler is entitled to inline static functions. If it did, likely it picked up the optmisation anyway as Hannes suggested. However, the other reason this might not be an optimisation (assuming the compiler doesn't inline the function) is you're passing an argument which can be offset computed. On all architectures, you have a fixed number of registers for passing function arguments, then we have to use the stack. Using the stack comes in far more expensive than computing an offset to an existing pointer. Even if you're still in registers, the offset now has to be computed and stored and the compiler loses track of the relation. The bottom line is that adding an extra argument for a value which can be offset computed is rarely a win. James, When I did this, I was mostly concerned about the cost of reestablishing state that was already known. So, even with the function being in-lined, I felt the cost of reestablishing state that was already known is unnecessary. In this particular case, I did not change the number of arguments that were being passed; I just changed the type of one of them - instead of passing struct hv_device *, I am now passing struct storvsc_device *. In the current code, we are using struct hv_device * to establish a pointer to struct storvsc_device * via the function get_in_stor_device(). This pattern currently exists in the call chain from the interrupt handler - storvsc_on_channel_callback(). While the compiler is smart enough to inline both get_in_stor_device() as well as many of the static functions in the call chain from storvsc_on_channel_callback(), looking at the assembled code, the compiler is repeatedly inlining the call to get_in_stor_device() and this clearly is less than optimal. Which means you actually checked the compiler output, and it made a difference. That's all I wanted to know, as it's not immediately clear from the patch. So: Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes ReineckezSeries & Storage h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg) -- 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
