Re: [PATCH -next] scsi: bnx2i: remove set but not used variable 'cid_num'

2018-11-11 Thread Javali, Nilesh 



On 11/12/18, 7:24 AM, "YueHaibing"  wrote:

>External Email
>
>Fixes gcc '-Wunused-but-set-variable' warning:
>
>drivers/scsi/bnx2i/bnx2i_hwi.c: In function 'bnx2i_process_ofld_cmpl':
>drivers/scsi/bnx2i/bnx2i_hwi.c:2430:6: warning:
> variable 'cid_num' set but not used [-Wunused-but-set-variable]
>
>It never used since commit
>  cf4e6363859d ("[SCSI] bnx2i: Add bnx2i iSCSI driver.")
>
>Signed-off-by: YueHaibing 
>---
> drivers/scsi/bnx2i/bnx2i_hwi.c | 2 --
> 1 file changed, 2 deletions(-)
>
>diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c
>b/drivers/scsi/bnx2i/bnx2i_hwi.c
>index 6bad268..91f5316 100644
>--- a/drivers/scsi/bnx2i/bnx2i_hwi.c
>+++ b/drivers/scsi/bnx2i/bnx2i_hwi.c
>@@ -2427,7 +2427,6 @@ static void bnx2i_process_ofld_cmpl(struct
>bnx2i_hba *hba,
> {
>u32 cid_addr;
>struct bnx2i_endpoint *ep;
>-   u32 cid_num;
>
>ep = bnx2i_find_ep_in_ofld_list(hba, ofld_kcqe->iscsi_conn_id);
>if (!ep) {
>@@ -2462,7 +2461,6 @@ static void bnx2i_process_ofld_cmpl(struct
>bnx2i_hba *hba,
>} else {
>ep->state = EP_STATE_OFLD_COMPL;
>cid_addr = ofld_kcqe->iscsi_conn_context_id;
>-   cid_num = bnx2i_get_cid_num(ep);
>ep->ep_cid = cid_addr;
>ep->qp.ctx_base = NULL;
>}
>

Thanks.
Acked-by: Nilesh Javali

Re: [PATCH] qedi: Fix a potential buffer overflow

2018-07-27 Thread Javali, Nilesh 


On 7/27/18, 2:40 AM, "Bart Van Assche"  wrote:

>External Email
>
>Tell snprintf() to store at most 255 characters in the output buffer
>instead of 256. This patch avoids that smatch reports the following
>warning:
>
>drivers/scsi/qedi/qedi_main.c:891: qedi_get_boot_tgt_info() error:
>snprintf() is printing too much 256 vs 255
>
>Signed-off-by: Bart Van Assche 
>Cc: 
>Cc: 
>---
> drivers/scsi/qedi/qedi_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
>index 682f3ce31014..ea62180d9ec8 100644
>--- a/drivers/scsi/qedi/qedi_main.c
>+++ b/drivers/scsi/qedi/qedi_main.c
>@@ -888,7 +888,7 @@ static void qedi_get_boot_tgt_info(struct
>nvm_iscsi_block *block,
>ipv6_en = !!(block->generic.ctrl_flags &
> NVM_ISCSI_CFG_GEN_IPV6_ENABLED);
>
>-   snprintf(tgt->iscsi_name, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN,
>"%s\n",
>+   snprintf(tgt->iscsi_name, sizeof(tgt->iscsi_name), "%s\n",
> block->target[index].target_name.byte);
>
>tgt->ipv6_en = ipv6_en;
>--
>2.18.0
>

Thanks,

Acked-by: Nilesh Javali

Re: [PATCH] qedi: Fix static checker warning

2018-06-26 Thread Javali, Nilesh 



On 6/25/18, 8:31 PM, "Bart Van Assche"  wrote:

>External Email
>
>On 06/25/18 05:32, Nilesh Javali wrote:
>> This patch fixes the static checker warning,
>>
>> drivers/scsi/qedi/qedi_main.c:891 qedi_get_boot_tgt_info()
>>  error: snprintf() is printing too much 256 vs 255
>
>Which static checker produced this warning?
>
>> Signed-off-by: Nilesh Javali 
>> ---
>>   drivers/scsi/qedi/qedi_main.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/qedi/qedi_main.c
>>b/drivers/scsi/qedi/qedi_main.c
>> index cf274a7..85491da 100644
>> --- a/drivers/scsi/qedi/qedi_main.c
>> +++ b/drivers/scsi/qedi/qedi_main.c
>> @@ -888,8 +888,8 @@ static void qedi_get_boot_tgt_info(struct
>>nvm_iscsi_block *block,
>>   ipv6_en = !!(block->generic.ctrl_flags &
>>NVM_ISCSI_CFG_GEN_IPV6_ENABLED);
>>
>> - snprintf(tgt->iscsi_name, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN,
>>"%s\n",
>> -  block->target[index].target_name.byte);
>> + sprintf(tgt->iscsi_name, "%.*s\n",
>>NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN,
>> + block->target[index].target_name.byte);
>>
>>   tgt->ipv6_en = ipv6_en;
>
>Since sizeof(tgt->iscsi_name) == 255, since
>NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN == 256 and since
>sizeof(block->target[index].target_name.byte) == 256, I think you are
>making a potential buffer overflow worse instead of just suppressing a
>static checker warning.
>
>Bart.

I will send another patch set fixing the sizeof target name
(tgt->iscsi_name) along with the above fix.
Thanks for the review.

Thanks,
Nilesh

Re: [PATCH V2 1/2] qedi: Fix truncation of CHAP name and secret

2018-02-07 Thread Javali, Nilesh 

On 2/6/18, 8:53 PM, "Bart Van Assche"  wrote:

>On Tue, 2018-02-06 at 05:12 -0800, Nilesh Javali wrote:
>> From: Andrew Vasquez 
>> 
>> The data in NVRAM is not guaranteed to be NUL terminated.
>> Copy the data upto the element size or to the first NUL
>> in the byte-stream and then append a NUL.
>> 
>> Signed-off-by: Andrew Vasquez 
>> Signed-off-by: Nilesh Javali 
>> ---
>>  drivers/scsi/qedi/qedi_main.c | 45
>>+++
>>  1 file changed, 33 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/scsi/qedi/qedi_main.c
>>b/drivers/scsi/qedi/qedi_main.c
>> index 8808f0d..f3dd438 100644
>> --- a/drivers/scsi/qedi/qedi_main.c
>> +++ b/drivers/scsi/qedi/qedi_main.c
>> @@ -1705,6 +1705,27 @@ void qedi_reset_host_mtu(struct qedi_ctx *qedi,
>>u16 mtu)
>>  qedi_ops->ll2->start(qedi->cdev, );
>>  }
>>  
>> +static ssize_t
>> +qedi_show_copy_data(char *buf, size_t size, u8 *data)
>> +{
>> +size_t i;
>> +
>> +if (!data)
>> +return sprintf(buf, "\n");
>> +
>> +/*
>> + * Data not guaranteed to be NUL terminated. Copy until NUL found or
>> + * complete copy done.
>> + */
>> +for (i = 0; i < size && data[i]; i++)
>> +buf[i] = data[i];
>> +/* Data copy complete, append NEWLINE and NUL terminator. */
>> +buf[i] = '\n';
>> +buf[i + 1] = '\0';
>> +return strlen(buf);
>> +}
>
>Can the body of the above function be changed into the following, which
>is much
>shorter?
>
>sprintf(buf, "%.*s", (int)size, data ? : "")

This looks clean and shorter. I will send the updated patch.



Thanks,
Nilesh

Re: [PATCH] qedi: Fix truncation of name and secret

2018-02-05 Thread Javali, Nilesh 

On 2/6/18, 12:51 AM, "Lee Duncan"  wrote:

>On 02/05/2018 11:15 AM, Lee Duncan wrote:
>> On 01/31/2018 10:57 PM, Nilesh Javali wrote:
>>> Adjust the NULL byte added by snprintf.
>>>
>>> Signed-off-by: Nilesh Javali 
>>> ---
>>>  drivers/scsi/qedi/qedi_main.c | 12 ++--
>>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/scsi/qedi/qedi_main.c
>>>b/drivers/scsi/qedi/qedi_main.c
>>> index 34a..cf8badb 100644
>>> --- a/drivers/scsi/qedi/qedi_main.c
>>> +++ b/drivers/scsi/qedi/qedi_main.c
>>> @@ -1840,7 +1840,7 @@ static ssize_t qedi_show_boot_ini_info(void
>>>*data, int type, char *buf)
>>>  
>>> switch (type) {
>>> case ISCSI_BOOT_INI_INITIATOR_NAME:
>>> -   rc = snprintf(str, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN, "%s\n",
>>> +   rc = snprintf(str, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN + 1, "%s",
>>>   initiator->initiator_name.byte);
>>> break;
>>> default:
>>> @@ -1908,7 +1908,7 @@ static umode_t qedi_ini_get_attr_visibility(void
>>>*data, int type)
>>>  
>>> switch (type) {
>>> case ISCSI_BOOT_TGT_NAME:
>>> -   rc = snprintf(str, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN, "%s\n",
>>> +   rc = snprintf(str, NVM_ISCSI_CFG_ISCSI_NAME_MAX_LEN + 1, "%s",
>>>   block->target[idx].target_name.byte);
>>> break;
>>> case ISCSI_BOOT_TGT_IP_ADDR:
>>> @@ -1930,19 +1930,19 @@ static umode_t
>>>qedi_ini_get_attr_visibility(void *data, int type)
>>>   block->target[idx].lun.value[0]);
>>> break;
>>> case ISCSI_BOOT_TGT_CHAP_NAME:
>>> -   rc = snprintf(str, NVM_ISCSI_CFG_CHAP_NAME_MAX_LEN, "%s\n",
>>> +   rc = snprintf(str, NVM_ISCSI_CFG_CHAP_NAME_MAX_LEN + 1, "%s",
>>>   chap_name);
>>> break;
>>> case ISCSI_BOOT_TGT_CHAP_SECRET:
>>> -   rc = snprintf(str, NVM_ISCSI_CFG_CHAP_PWD_MAX_LEN, "%s\n",
>>> +   rc = snprintf(str, NVM_ISCSI_CFG_CHAP_PWD_MAX_LEN + 1, "%s",
>>>   chap_secret);
>>> break;
>>> case ISCSI_BOOT_TGT_REV_CHAP_NAME:
>>> -   rc = snprintf(str, NVM_ISCSI_CFG_CHAP_NAME_MAX_LEN, "%s\n",
>>> +   rc = snprintf(str, NVM_ISCSI_CFG_CHAP_NAME_MAX_LEN + 1, "%s",
>>>   mchap_name);
>>> break;
>>> case ISCSI_BOOT_TGT_REV_CHAP_SECRET:
>>> -   rc = snprintf(str, NVM_ISCSI_CFG_CHAP_PWD_MAX_LEN, "%s\n",
>>> +   rc = snprintf(str, NVM_ISCSI_CFG_CHAP_PWD_MAX_LEN + 1, "%s",
>>>   mchap_secret);
>>> break;
>>> case ISCSI_BOOT_TGT_FLAGS:
>>>
>> 
>> Reviewed-by: Lee Duncan 
>> 
>
>Assuming you address Bart's comments.
>-- 
>Lee Duncan
>SUSE Labs

Bart, Lee,

Thanks for the review and comments. Please ignore the current patch.

The data in NVRAM is not guaranteed to be NUL terminated.
Hence in V2 solution, the data would be copied upto the element size or to
the first NUL
in the byte-stream and then append a NUL.

I would post V2 of the solution shortly.


Thanks,
Nilesh

Re: [PATCH] scsi: qedi: select CONFIG_ISCSI_BOOT_SYSFS

2017-07-24 Thread Javali, Nilesh 

On 21/07/17, 9:41 PM, "Arnd Bergmann"  wrote:

>Without the base library support, we get a link failure
>
>drivers/scsi/qedi/qedi_main.o: In function `__qedi_probe.constprop.0':
>qedi_main.c:(.text+0x2d8e): undefined reference to
>`iscsi_boot_create_target'
>qedi_main.c:(.text+0x2dee): undefined reference to
>`iscsi_boot_create_initiator'
>qedi_main.c:(.text+0x2e1c): undefined reference to
>`iscsi_boot_create_ethernet'
>
>This selects the Kconfig symbol like the other two users of that
>module do.
>
>Fixes: c57ec8fb7c02 ("scsi: qedi: Add support for Boot from SAN over
>iSCSI offload")
>Signed-off-by: Arnd Bergmann 
>---
> drivers/scsi/qedi/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/drivers/scsi/qedi/Kconfig b/drivers/scsi/qedi/Kconfig
>index 21331453db7b..8deb8723c4dd 100644
>--- a/drivers/scsi/qedi/Kconfig
>+++ b/drivers/scsi/qedi/Kconfig
>@@ -2,6 +2,7 @@ config QEDI
>   tristate "QLogic QEDI 25/40/100Gb iSCSI Initiator Driver Support"
>   depends on PCI && SCSI && UIO
>   depends on QED
>+  select ISCSI_BOOT_SYSFS
>   select SCSI_ISCSI_ATTRS
>   select QED_LL2
>   select QED_ISCSI
>-- 
>2.9.0
>

NACK. The fix already posted to address this issue,
http://marc.info/?l=linux-scsi=150045528332067=2

Thanks,
Nilesh

Re: [PATCH 1/1] qedi: Add support for offload iSCSI Boot

2017-06-27 Thread Javali, Nilesh 


On 26/06/17, 10:29 PM, "Martin K. Petersen" 
wrote:

>
>Nilesh,
>
>> This patch adds support for offload iSCSI boot (Boot from SAN
>> over iSCSI offload).
>>
>> The dependent qed patches for this support are,
>> - qed: Support NVM-image reading API
>> - qed: Share additional information with qedf
>
>You had tagged this for 4.12/scsi-fixes but the above patches are not
>present in 4.12. So I presume this would be a candidate for a second
>stage 4.13 merge?
Yes.

>
>Patch looks OK to me, but please resubmit with a slightly more verbose
>description of the changes and how they work.
Done. Resubmitted the patch.

Thanks,
Nilesh

Re: [PATCH v2] scsi: qla4xxx: Use dma_pool_zalloc

2017-01-13 Thread Javali, Nilesh 


On 02/01/17, 11:10 AM, "Souptick Joarder"  wrote:

>Hi Nilesh,
>
>On Thu, Dec 22, 2016 at 5:42 PM, Souptick Joarder 
>wrote:
>> We should use dma_pool_zalloc instead of dma_pool_alloc/memset
>>
>> Signed-off-by: Souptick joarder 
>> ---
>> v2 changes:
>>- Address comment from Nilesh to make same change in
>>  all applicable places inside qla4xxx source
>>
>>  drivers/scsi/qla4xxx/ql4_mbx.c | 6 ++
>>  drivers/scsi/qla4xxx/ql4_os.c  | 4 +---
>>  2 files changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/scsi/qla4xxx/ql4_mbx.c
>>b/drivers/scsi/qla4xxx/ql4_mbx.c
>> index c291fdf..8f97839 100644
>> --- a/drivers/scsi/qla4xxx/ql4_mbx.c
>> +++ b/drivers/scsi/qla4xxx/ql4_mbx.c
>> @@ -1587,12 +1587,11 @@ int qla4xxx_get_chap(struct scsi_qla_host *ha,
>>char *username, char *password,
>> struct ql4_chap_table *chap_table;
>> dma_addr_t chap_dma;
>>
>> -   chap_table = dma_pool_alloc(ha->chap_dma_pool, GFP_KERNEL,
>>_dma);
>> +   chap_table = dma_pool_zalloc(ha->chap_dma_pool, GFP_KERNEL,
>>_dma);
>> if (chap_table == NULL)
>> return -ENOMEM;
>>
>> chap_size = sizeof(struct ql4_chap_table);
>> -   memset(chap_table, 0, chap_size);
>>
>> if (is_qla40XX(ha))
>> offset = FLASH_CHAP_OFFSET | (idx * chap_size);
>> @@ -1651,13 +1650,12 @@ int qla4xxx_set_chap(struct scsi_qla_host *ha,
>>char *username, char *password,
>> uint32_t chap_size = 0;
>> dma_addr_t chap_dma;
>>
>> -   chap_table = dma_pool_alloc(ha->chap_dma_pool, GFP_KERNEL,
>>_dma);
>> +   chap_table = dma_pool_zalloc(ha->chap_dma_pool, GFP_KERNEL,
>>_dma);
>> if (chap_table == NULL) {
>> ret =  -ENOMEM;
>> goto exit_set_chap;
>> }
>>
>> -   memset(chap_table, 0, sizeof(struct ql4_chap_table));
>> if (bidi)
>> chap_table->flags |= BIT_6; /* peer */
>> else
>> diff --git a/drivers/scsi/qla4xxx/ql4_os.c
>>b/drivers/scsi/qla4xxx/ql4_os.c
>> index 01c3610..0c91c89 100644
>> --- a/drivers/scsi/qla4xxx/ql4_os.c
>> +++ b/drivers/scsi/qla4xxx/ql4_os.c
>> @@ -825,12 +825,10 @@ static int qla4xxx_delete_chap(struct Scsi_Host
>>*shost, uint16_t chap_tbl_idx)
>> uint32_t chap_size;
>> int ret = 0;
>>
>> -   chap_table = dma_pool_alloc(ha->chap_dma_pool, GFP_KERNEL,
>>_dma);
>> +   chap_table = dma_pool_zalloc(ha->chap_dma_pool, GFP_KERNEL,
>>_dma);
>> if (chap_table == NULL)
>> return -ENOMEM;
>>
>> -   memset(chap_table, 0, sizeof(struct ql4_chap_table));
>> -
>> if (is_qla80XX(ha))
>> max_chap_entries = (ha->hw.flt_chap_size / 2) /
>>sizeof(struct ql4_chap_table);
>> --
>> 1.9.1
>>
>
>Any further comment on this patch ?

Any reason for handling this only for CHAP related code and not take care
at all other places within ql4_os.c and ql4_init.c.
According to me the change should be consistent across all the affecting
files unless you think otherwise.

Thanks,
Nilesh

--
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] scsi/qla4: comments correction

2016-12-22 Thread Javali, Nilesh 


On 19/12/16, 11:50 AM, "linux-scsi-ow...@vger.kernel.org on behalf of Cao
jin"  wrote:

>Signed-off-by: Cao jin 
>---
> drivers/scsi/qla4xxx/ql4_os.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
>index 01c3610a60cf..fc60e0a1043e 100644
>--- a/drivers/scsi/qla4xxx/ql4_os.c
>+++ b/drivers/scsi/qla4xxx/ql4_os.c
>@@ -9546,15 +9546,15 @@ static int qla4xxx_host_reset(struct Scsi_Host
>*shost, int reset_type)
>  * driver calls the following device driver's callbacks
>  *
>  * - Fatal Errors - link_reset
>- * - Non-Fatal Errors - driver's pci_error_detected() which
>+ * - Non-Fatal Errors - driver's error_detected() which
>  * returns CAN_RECOVER, NEED_RESET or DISCONNECT.
>  *
>  * PCI AER driver calls
>- * CAN_RECOVER - driver's pci_mmio_enabled(), mmio_enabled
>+ * CAN_RECOVER - driver's mmio_enabled(), mmio_enabled()
>  *   returns RECOVERED or NEED_RESET if fw_hung
>  * NEED_RESET - driver's slot_reset()
>  * DISCONNECT - device is dead & cannot recover
>- * RECOVERED - driver's pci_resume()
>+ * RECOVERED - driver's resume()
>  */
> static pci_ers_result_t
> qla4xxx_pci_error_detected(struct pci_dev *pdev, pci_channel_state_t
>state)
>-- 
>2.1.0

Acked-by: Nilesh Javali 

--
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] scsi: qla4xxx: Use dma_pool_zalloc

2016-12-21 Thread Javali, Nilesh 


On 13/12/16, 6:35 PM, "Souptick Joarder" <jrdr.li...@gmail.com> wrote:

>On Tue, Dec 13, 2016 at 4:11 PM, Javali, Nilesh
><nilesh.jav...@cavium.com> wrote:
>>
>>
>> On 12/12/16, 10:16 AM, "linux-scsi-ow...@vger.kernel.org on behalf of
>> Souptick Joarder" <linux-scsi-ow...@vger.kernel.org on behalf of
>> jrdr.li...@gmail.com> wrote:
>>
>>>On Wed, Dec 7, 2016 at 1:53 AM, Souptick Joarder <jrdr.li...@gmail.com>
>>>wrote:
>>>> We should use dma_pool_zalloc instead of dma_pool_alloc/memset
>>>>
>>>> Signed-off-by: Souptick joarder <jrdr.li...@gmail.com>
>>>> ---
>>>>  drivers/scsi/qla4xxx/ql4_mbx.c | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/qla4xxx/ql4_mbx.c
>>>>b/drivers/scsi/qla4xxx/ql4_mbx.c
>>>> index c291fdf..f2edfd7 100644
>>>> --- a/drivers/scsi/qla4xxx/ql4_mbx.c
>>>> +++ b/drivers/scsi/qla4xxx/ql4_mbx.c
>>>> @@ -1587,12 +1587,11 @@ int qla4xxx_get_chap(struct scsi_qla_host *ha,
>>>>char *username, char *password,
>>>> struct ql4_chap_table *chap_table;
>>>> dma_addr_t chap_dma;
>>>>
>>>> -   chap_table = dma_pool_alloc(ha->chap_dma_pool, GFP_KERNEL,
>>>>_dma);
>>>> +   chap_table = dma_pool_zalloc(ha->chap_dma_pool, GFP_KERNEL,
>>>>_dma);
>>>> if (chap_table == NULL)
>>>> return -ENOMEM;
>>>>
>>>> chap_size = sizeof(struct ql4_chap_table);
>>>> -   memset(chap_table, 0, chap_size);
>>>>
>>>> if (is_qla40XX(ha))
>>>> offset = FLASH_CHAP_OFFSET | (idx * chap_size);
>>>> --
>>>> 1.9.1
>>>>
>>>
>>>Any comment on this patch?
>>
>> There are multiple other places where dma_pool_alloc needs to be
>>replaced
>> by dma_pool_zalloc.
>
>
> are you asking to do this for SCSI subsystem?
> If yes, I can do that.
>
>>
>> Can you please take care within a single patch.
>
> But the same changes are applicable in other subsystems as well.
> I think I shouldn't send those changes in a single patch as
>maintainers are different
> for different modules.

I'm referring to multiple other files within qla4xxx source which uses
dma_pool_alloc.
The current patch takes care only of ql4_mbx.c and qla4xxx_get_chap
function.
There is qla4xxx_set_chap function as well. Also ql4_init.c and ql4_os.c
files also use dma_pool_alloc.
I was requesting to make all these changes related to qla4xxx module
within a single patch.

Thanks,
Nilesh

--
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] scsi: qla4xxx: Use dma_pool_zalloc

2016-12-13 Thread Javali, Nilesh 


On 12/12/16, 10:16 AM, "linux-scsi-ow...@vger.kernel.org on behalf of
Souptick Joarder"  wrote:

>On Wed, Dec 7, 2016 at 1:53 AM, Souptick Joarder 
>wrote:
>> We should use dma_pool_zalloc instead of dma_pool_alloc/memset
>>
>> Signed-off-by: Souptick joarder 
>> ---
>>  drivers/scsi/qla4xxx/ql4_mbx.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/qla4xxx/ql4_mbx.c
>>b/drivers/scsi/qla4xxx/ql4_mbx.c
>> index c291fdf..f2edfd7 100644
>> --- a/drivers/scsi/qla4xxx/ql4_mbx.c
>> +++ b/drivers/scsi/qla4xxx/ql4_mbx.c
>> @@ -1587,12 +1587,11 @@ int qla4xxx_get_chap(struct scsi_qla_host *ha,
>>char *username, char *password,
>> struct ql4_chap_table *chap_table;
>> dma_addr_t chap_dma;
>>
>> -   chap_table = dma_pool_alloc(ha->chap_dma_pool, GFP_KERNEL,
>>_dma);
>> +   chap_table = dma_pool_zalloc(ha->chap_dma_pool, GFP_KERNEL,
>>_dma);
>> if (chap_table == NULL)
>> return -ENOMEM;
>>
>> chap_size = sizeof(struct ql4_chap_table);
>> -   memset(chap_table, 0, chap_size);
>>
>> if (is_qla40XX(ha))
>> offset = FLASH_CHAP_OFFSET | (idx * chap_size);
>> --
>> 1.9.1
>>
>
>Any comment on this patch?

There are multiple other places where dma_pool_alloc needs to be replaced
by dma_pool_zalloc.

Can you please take care within a single patch.

Thanks,
Nilesh

--
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] qla4xxx: switch to pci_alloc_irq_vectors

2016-12-06 Thread Javali, Nilesh 

On 06/12/16, 7:26 PM, "Christoph Hellwig"  wrote:

>And simplify the MSI-X logic in general - just request the two
>vectors directly instead of going through an indirection table.
>
>Signed-off-by: Christoph Hellwig 
>---
> drivers/scsi/qla4xxx/ql4_def.h  | 18 +
> drivers/scsi/qla4xxx/ql4_glbl.h |  1 -
> drivers/scsi/qla4xxx/ql4_isr.c  | 27 +
> drivers/scsi/qla4xxx/ql4_nx.c   | 89
>+++--
> 4 files changed, 37 insertions(+), 98 deletions(-)
>
>diff --git a/drivers/scsi/qla4xxx/ql4_def.h
>b/drivers/scsi/qla4xxx/ql4_def.h
>index a7cfc27..aeebefb 100644
>--- a/drivers/scsi/qla4xxx/ql4_def.h
>+++ b/drivers/scsi/qla4xxx/ql4_def.h
>@@ -409,18 +409,9 @@ struct qla4_8xxx_legacy_intr_set {
> 
> /* MSI-X Support */
> 
>-#define QLA_MSIX_DEFAULT  0x00
>-#define QLA_MSIX_RSP_Q0x01
>-
>+#define QLA_MSIX_DEFAULT  0
>+#define QLA_MSIX_RSP_Q1
> #define QLA_MSIX_ENTRIES  2
>-#define QLA_MIDX_DEFAULT  0
>-#define QLA_MIDX_RSP_Q1
>-
>-struct ql4_msix_entry {
>-  int have_irq;
>-  uint16_t msix_vector;
>-  uint16_t msix_entry;
>-};
> 
> /*
>  * ISP Operations
>@@ -572,9 +563,6 @@ struct scsi_qla_host {
> #define AF_IRQ_ATTACHED   10 /* 0x0400 */
> #define AF_DISABLE_ACB_COMPLETE   11 /* 0x0800 */
> #define AF_HA_REMOVAL 12 /* 0x1000 */
>-#define AF_INTx_ENABLED   15 /* 0x8000 */
>-#define AF_MSI_ENABLED16 /* 0x0001 */
>-#define AF_MSIX_ENABLED   17 /* 0x0002 */
> #define AF_MBOX_COMMAND_NOPOLL18 /* 0x0004 */
> #define AF_FW_RECOVERY19 /* 0x0008 */
> #define AF_EEH_BUSY   20 /* 0x0010 */
>@@ -762,8 +750,6 @@ struct scsi_qla_host {
>   struct isp_operations *isp_ops;
>   struct ql82xx_hw_data hw;
> 
>-  struct ql4_msix_entry msix_entries[QLA_MSIX_ENTRIES];
>-
>   uint32_t nx_dev_init_timeout;
>   uint32_t nx_reset_timeout;
>   void *fw_dump;
>diff --git a/drivers/scsi/qla4xxx/ql4_glbl.h
>b/drivers/scsi/qla4xxx/ql4_glbl.h
>index 2559144..bce96a5 100644
>--- a/drivers/scsi/qla4xxx/ql4_glbl.h
>+++ b/drivers/scsi/qla4xxx/ql4_glbl.h
>@@ -134,7 +134,6 @@ int qla4_8xxx_get_flash_info(struct scsi_qla_host
>*ha);
> void qla4_82xx_enable_intrs(struct scsi_qla_host *ha);
> void qla4_82xx_disable_intrs(struct scsi_qla_host *ha);
> int qla4_8xxx_enable_msix(struct scsi_qla_host *ha);
>-void qla4_8xxx_disable_msix(struct scsi_qla_host *ha);
> irqreturn_t qla4_8xxx_msi_handler(int irq, void *dev_id);
> irqreturn_t qla4_8xxx_default_intr_handler(int irq, void *dev_id);
> irqreturn_t qla4_8xxx_msix_rsp_q(int irq, void *dev_id);
>diff --git a/drivers/scsi/qla4xxx/ql4_isr.c
>b/drivers/scsi/qla4xxx/ql4_isr.c
>index 4f9c0f2..d2cd33d 100644
>--- a/drivers/scsi/qla4xxx/ql4_isr.c
>+++ b/drivers/scsi/qla4xxx/ql4_isr.c
>@@ -1107,7 +1107,7 @@ static void qla4_82xx_spurious_interrupt(struct
>scsi_qla_host *ha,
>   DEBUG2(ql4_printk(KERN_INFO, ha, "Spurious Interrupt\n"));
>   if (is_qla8022(ha)) {
>   writel(0, >qla4_82xx_reg->host_int);
>-  if (test_bit(AF_INTx_ENABLED, >flags))
>+  if (!ha->pdev->msi_enabled && !ha->pdev->msix_enabled)
>   qla4_82xx_wr_32(ha, ha->nx_legacy_intr.tgt_mask_reg,
>   0xfbff);
>   }
>@@ -1564,19 +1564,18 @@ int qla4xxx_request_irqs(struct scsi_qla_host *ha)
> 
> try_msi:
>   /* Trying MSI */
>-  ret = pci_enable_msi(ha->pdev);
>-  if (!ret) {
>+  ret = pci_alloc_irq_vectors(ha->pdev, 1, 1, PCI_IRQ_MSI);
>+  if (ret > 0) {
>   ret = request_irq(ha->pdev->irq, qla4_8xxx_msi_handler,
>   0, DRIVER_NAME, ha);
>   if (!ret) {
>   DEBUG2(ql4_printk(KERN_INFO, ha, "MSI: Enabled.\n"));
>-  set_bit(AF_MSI_ENABLED, >flags);
>   goto irq_attached;
>   } else {
>   ql4_printk(KERN_WARNING, ha,
>   "MSI: Failed to reserve interrupt %d "
>   "already in use.\n", ha->pdev->irq);
>-  pci_disable_msi(ha->pdev);
>+  pci_free_irq_vectors(ha->pdev);
>   }
>   }
> 
>@@ -1592,7 +1591,6 @@ int qla4xxx_request_irqs(struct scsi_qla_host *ha)
>   IRQF_SHARED, DRIVER_NAME, ha);
>   if (!ret) {
>   DEBUG2(ql4_printk(KERN_INFO, ha, "INTx: Enabled.\n"));
>-  set_bit(AF_INTx_ENABLED, >flags);
>   goto irq_attached;
> 
>   } else {
>@@ -1614,14 +1612,11 @@ int qla4xxx_request_irqs(struct scsi_qla_host *ha)
> 
> void qla4xxx_free_irqs(struct scsi_qla_host *ha)
> {
>-  if (test_and_clear_bit(AF_IRQ_ATTACHED, >flags)) {
>-  if (test_bit(AF_MSIX_ENABLED, >flags)) {
>-

Re: [PATCH] qla4xxx: switch to pci_alloc_irq_vectors

2016-12-06 Thread Javali, Nilesh 
Please see comments inline.

Thanks,
Nilesh

On 18/11/16, 12:45 PM, "Christoph Hellwig"  wrote:

>And simplify the MSI-X logic in general - just request the two
>vectors directly instead of going through an indirection table.
>
>Signed-off-by: Christoph Hellwig 
>---
> drivers/scsi/qla4xxx/ql4_def.h  | 18 +
> drivers/scsi/qla4xxx/ql4_glbl.h |  1 -
> drivers/scsi/qla4xxx/ql4_isr.c  | 25 +---
> drivers/scsi/qla4xxx/ql4_nx.c   | 87
>+++--
> 4 files changed, 35 insertions(+), 96 deletions(-)
>
>diff --git a/drivers/scsi/qla4xxx/ql4_def.h
>b/drivers/scsi/qla4xxx/ql4_def.h
>index a7cfc27..aeebefb 100644
>--- a/drivers/scsi/qla4xxx/ql4_def.h
>+++ b/drivers/scsi/qla4xxx/ql4_def.h
>@@ -409,18 +409,9 @@ struct qla4_8xxx_legacy_intr_set {
> 
> /* MSI-X Support */
> 
>-#define QLA_MSIX_DEFAULT  0x00
>-#define QLA_MSIX_RSP_Q0x01
>-
>+#define QLA_MSIX_DEFAULT  0
>+#define QLA_MSIX_RSP_Q1
> #define QLA_MSIX_ENTRIES  2
>-#define QLA_MIDX_DEFAULT  0
>-#define QLA_MIDX_RSP_Q1
>-
>-struct ql4_msix_entry {
>-  int have_irq;
>-  uint16_t msix_vector;
>-  uint16_t msix_entry;
>-};
> 
> /*
>  * ISP Operations
>@@ -572,9 +563,6 @@ struct scsi_qla_host {
> #define AF_IRQ_ATTACHED   10 /* 0x0400 */
> #define AF_DISABLE_ACB_COMPLETE   11 /* 0x0800 */
> #define AF_HA_REMOVAL 12 /* 0x1000 */
>-#define AF_INTx_ENABLED   15 /* 0x8000 */
>-#define AF_MSI_ENABLED16 /* 0x0001 */
>-#define AF_MSIX_ENABLED   17 /* 0x0002 */
> #define AF_MBOX_COMMAND_NOPOLL18 /* 0x0004 */
> #define AF_FW_RECOVERY19 /* 0x0008 */
> #define AF_EEH_BUSY   20 /* 0x0010 */
>@@ -762,8 +750,6 @@ struct scsi_qla_host {
>   struct isp_operations *isp_ops;
>   struct ql82xx_hw_data hw;
> 
>-  struct ql4_msix_entry msix_entries[QLA_MSIX_ENTRIES];
>-
>   uint32_t nx_dev_init_timeout;
>   uint32_t nx_reset_timeout;
>   void *fw_dump;
>diff --git a/drivers/scsi/qla4xxx/ql4_glbl.h
>b/drivers/scsi/qla4xxx/ql4_glbl.h
>index 2559144..bce96a5 100644
>--- a/drivers/scsi/qla4xxx/ql4_glbl.h
>+++ b/drivers/scsi/qla4xxx/ql4_glbl.h
>@@ -134,7 +134,6 @@ int qla4_8xxx_get_flash_info(struct scsi_qla_host
>*ha);
> void qla4_82xx_enable_intrs(struct scsi_qla_host *ha);
> void qla4_82xx_disable_intrs(struct scsi_qla_host *ha);
> int qla4_8xxx_enable_msix(struct scsi_qla_host *ha);
>-void qla4_8xxx_disable_msix(struct scsi_qla_host *ha);
> irqreturn_t qla4_8xxx_msi_handler(int irq, void *dev_id);
> irqreturn_t qla4_8xxx_default_intr_handler(int irq, void *dev_id);
> irqreturn_t qla4_8xxx_msix_rsp_q(int irq, void *dev_id);
>diff --git a/drivers/scsi/qla4xxx/ql4_isr.c
>b/drivers/scsi/qla4xxx/ql4_isr.c
>index 4f9c0f2..ff89bba 100644
>--- a/drivers/scsi/qla4xxx/ql4_isr.c
>+++ b/drivers/scsi/qla4xxx/ql4_isr.c
>@@ -1107,7 +1107,7 @@ static void qla4_82xx_spurious_interrupt(struct
>scsi_qla_host *ha,
>   DEBUG2(ql4_printk(KERN_INFO, ha, "Spurious Interrupt\n"));
>   if (is_qla8022(ha)) {
>   writel(0, >qla4_82xx_reg->host_int);
>-  if (test_bit(AF_INTx_ENABLED, >flags))
>+  if (!ha->pdev->msi_enabled && !ha->pdev->msix_enabled)
>   qla4_82xx_wr_32(ha, ha->nx_legacy_intr.tgt_mask_reg,
>   0xfbff);
>   }
>@@ -1564,19 +1564,18 @@ int qla4xxx_request_irqs(struct scsi_qla_host *ha)
> 
> try_msi:
>   /* Trying MSI */
>-  ret = pci_enable_msi(ha->pdev);
>+  ret = pci_alloc_irq_vectors(ha->pdev, 1, 1, PCI_IRQ_MSI);
>   if (!ret) {

Since pci_alloc_irq_vectors returns a negative error code upon error,
This should be if (ret).

>   ret = request_irq(ha->pdev->irq, qla4_8xxx_msi_handler,
>   0, DRIVER_NAME, ha);
>   if (!ret) {
>   DEBUG2(ql4_printk(KERN_INFO, ha, "MSI: Enabled.\n"));
>-  set_bit(AF_MSI_ENABLED, >flags);
>   goto irq_attached;
>   } else {
>   ql4_printk(KERN_WARNING, ha,
>   "MSI: Failed to reserve interrupt %d "
>   "already in use.\n", ha->pdev->irq);
>-  pci_disable_msi(ha->pdev);
>+  pci_free_irq_vectors(ha->pdev);
>   }
>   }
> 
>@@ -1592,7 +1591,6 @@ int qla4xxx_request_irqs(struct scsi_qla_host *ha)
>   IRQF_SHARED, DRIVER_NAME, ha);
>   if (!ret) {
>   DEBUG2(ql4_printk(KERN_INFO, ha, "INTx: Enabled.\n"));
>-  set_bit(AF_INTx_ENABLED, >flags);
>   goto irq_attached;
> 
>   } else {
>@@ -1614,14 +1612,11 @@ int qla4xxx_request_irqs(struct scsi_qla_host *ha)
> 
> void qla4xxx_free_irqs(struct scsi_qla_host *ha)
> {
>-

Re: [PATCH] qla4xxx: switch to pci_alloc_irq_vectors

2016-11-29 Thread Javali, Nilesh 
Hi Martin,

We would test this internally and then ACK within a week.

Thanks,
Nilesh

On 29/11/16, 10:15 PM, "Martin K. Petersen" 
wrote:

>> "Christoph" == Christoph Hellwig  writes:
>
>Christoph> And simplify the MSI-X logic in general - just request the
>Christoph> two vectors directly instead of going through an indirection
>Christoph> table.
>
>Vikas: Please review!
>
>-- 
>Martin K. Petersen Oracle Linux Engineering

--
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