RE: [PATCH v2] scsi: be2iscsi: Use kasprintf
> -Original Message-
> From: Himanshu Jha [mailto:himanshujha199...@gmail.com]
> Sent: Wednesday, October 11, 2017 9:06 PM
> To: j...@linux.vnet.ibm.com
> Cc: martin.peter...@oracle.com; linux-s...@vger.kernel.org; linux-
> ker...@vger.kernel.org; subbu.seethara...@broadcom.com;
> ketan.muka...@broadcom.com; jitendra.bhiv...@broadcom.com;
> Himanshu Jha
> Subject: [PATCH v2] scsi: be2iscsi: Use kasprintf
>
> Use kasprintf instead of combination of kmalloc and sprintf.
> Also, remove BEISCSI_MSI_NAME macro used to specify size of string as
> kasprintf handles size computations.
>
> Signed-off-by: Himanshu Jha
> ---
> v2:
>-remove the unnecessary macro BEISCSI_MSI_NAME.
>
> drivers/scsi/be2iscsi/be_main.c | 12 +---
> drivers/scsi/be2iscsi/be_main.h | 2 --
> 2 files changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/scsi/be2iscsi/be_main.c
b/drivers/scsi/be2iscsi/be_main.c
> index b4542e7..6a9ee0e 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -803,15 +803,14 @@ static int beiscsi_init_irqs(struct beiscsi_hba
*phba)
>
> if (pcidev->msix_enabled) {
> for (i = 0; i < phba->num_cpus; i++) {
> - phba->msi_name[i] = kzalloc(BEISCSI_MSI_NAME,
> - GFP_KERNEL);
> + phba->msi_name[i] = kasprintf(GFP_KERNEL,
> + "beiscsi_%02x_%02x",
> +
phba->shost->host_no, i);
> if (!phba->msi_name[i]) {
> ret = -ENOMEM;
> goto free_msix_irqs;
> }
>
> - sprintf(phba->msi_name[i], "beiscsi_%02x_%02x",
> - phba->shost->host_no, i);
> ret = request_irq(pci_irq_vector(pcidev, i),
> be_isr_msix, 0,
phba->msi_name[i],
> &phwi_context->be_eq[i]);
> @@ -824,13 +823,12 @@ static int beiscsi_init_irqs(struct beiscsi_hba
*phba)
> goto free_msix_irqs;
> }
> }
> - phba->msi_name[i] = kzalloc(BEISCSI_MSI_NAME,
> GFP_KERNEL);
> + phba->msi_name[i] = kasprintf(GFP_KERNEL,
> "beiscsi_mcc_%02x",
> + phba->shost->host_no);
> if (!phba->msi_name[i]) {
> ret = -ENOMEM;
> goto free_msix_irqs;
> }
> - sprintf(phba->msi_name[i], "beiscsi_mcc_%02x",
> - phba->shost->host_no);
> ret = request_irq(pci_irq_vector(pcidev, i), be_isr_mcc,
0,
> phba->msi_name[i], &phwi_context-
> >be_eq[i]);
> if (ret) {
> diff --git a/drivers/scsi/be2iscsi/be_main.h
b/drivers/scsi/be2iscsi/be_main.h
> index 81ce3ff..8166de5 100644
> --- a/drivers/scsi/be2iscsi/be_main.h
> +++ b/drivers/scsi/be2iscsi/be_main.h
> @@ -155,8 +155,6 @@
> #define PAGES_REQUIRED(x) \
> ((x < PAGE_SIZE) ? 1 : ((x + PAGE_SIZE - 1) / PAGE_SIZE))
>
> -#define BEISCSI_MSI_NAME 20 /* size of msi_name string */
> -
> #define MEM_DESCR_OFFSET 8
> #define BEISCSI_DEFQ_HDR 1
> #define BEISCSI_DEFQ_DATA 0
> --
> 2.7.4
Looks good.
- Thanks.
Reviewed-by: Jitendra Bhivare
RE: [PATCH 2/2] scsi: be2iscsi: set errno on error path
> -Original Message-
> From: Pan Bian [mailto:bianpan201...@163.com]
> Sent: Sunday, December 04, 2016 10:53 AM
> To: Subbu Seetharaman; Ketan Mukadam; Jitendra Bhivare; James E.J.
> Bottomley; Martin K. Petersen; linux-s...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; Pan Bian
> Subject: [PATCH 2/2] scsi: be2iscsi: set errno on error path
>
> From: Pan Bian
>
> Variable ret is reset in the loop, and its value will be 0 during the
second and
> after repeat of the loop. If pci_alloc_consistent() returns a NULL
pointer then, it
> will leaves with return value 0. 0 means no error, which is contrary to
the fact.
> This patches fixes the bug, explicitly assigning "-ENOMEM" to return
variable ret
> on the path that the call to
> pci_alloc_consistent() fails.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=188951
>
> Signed-off-by: Pan Bian
> ---
> drivers/scsi/be2iscsi/be_main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/be2iscsi/be_main.c
b/drivers/scsi/be2iscsi/be_main.c
> index b6c5791..b5112d6 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -3049,8 +3049,10 @@ static int beiscsi_create_eqs(struct beiscsi_hba
> *phba,
> eq_vaddress = pci_alloc_consistent(phba->pcidev,
> num_eq_pages *
PAGE_SIZE,
> &paddr);
> - if (!eq_vaddress)
> + if (!eq_vaddress) {
> + ret = -ENOMEM;
> goto create_eq_error;
> + }
>
> mem->va = eq_vaddress;
> ret = be_fill_queue(eq, phba->params.num_eq_entries,
> --
> 1.9.1
>
[JB] Thanks.
Reviewed-by: Jitendra Bhivare
RE: [PATCH 1/2] scsi: be2iscsi: set errno on error path
> -Original Message-
> From: Pan Bian [mailto:bianpan201...@163.com]
> Sent: Sunday, December 04, 2016 10:52 AM
> To: Subbu Seetharaman; Ketan Mukadam; Jitendra Bhivare; James E.J.
> Bottomley; Martin K. Petersen; linux-s...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; Pan Bian
> Subject: [PATCH 1/2] scsi: be2iscsi: set errno on error path
>
> From: Pan Bian
>
> Variable ret is reset in the loop, and its value will be 0 during the
second and
> after repeat of the loop. If pci_alloc_consistent() returns a NULL
pointer then, it
> will leaves with return value 0. 0 means no error, which is contrary to
the fact.
> This patches fixes the bug, explicitly assigning "-ENOMEM" to return
variable ret
> on the path that the call to
> pci_alloc_consistent() fails.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=188941
>
> Signed-off-by: Pan Bian
> ---
> drivers/scsi/be2iscsi/be_main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/be2iscsi/be_main.c
b/drivers/scsi/be2iscsi/be_main.c
> index d9239c2..b6c5791 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -3113,8 +3113,10 @@ static int beiscsi_create_cqs(struct beiscsi_hba
> *phba,
> cq_vaddress = pci_alloc_consistent(phba->pcidev,
> num_cq_pages *
PAGE_SIZE,
> &paddr);
> - if (!cq_vaddress)
> + if (!cq_vaddress) {
> + ret = -ENOMEM;
> goto create_cq_error;
> + }
>
> ret = be_fill_queue(cq, phba->params.num_cq_entries,
> sizeof(struct sol_cqe), cq_vaddress);
> --
> 1.9.1
[JB] Thanks.
Reviewed-by: Jitendra Bhivare
RE: [PATCH] scsi: be2iscsi: mark symbols static where possible
> -Original Message- > From: Baoyou Xie [mailto:baoyou@linaro.org] > Sent: Monday, September 26, 2016 5:31 PM > To: subbu.seethara...@broadcom.com; ketan.muka...@broadcom.com; > jitendra.bhiv...@broadcom.com; j...@linux.vnet.ibm.com; > martin.peter...@oracle.com > Cc: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org; a...@arndb.de; > baoyou@linaro.org; xie.bao...@zte.com.cn; han@zte.com.cn; > tang.qiang...@zte.com.cn > Subject: [PATCH] scsi: be2iscsi: mark symbols static where possible > > We get 6 warnings when building kernel with W=1: > drivers/scsi/be2iscsi/be_main.c:65:1: warning: no previous prototype for > 'beiscsi_log_enable_disp' [-Wmissing-prototypes] > drivers/scsi/be2iscsi/be_main.c:78:1: warning: no previous prototype for > 'beiscsi_log_enable_change' [-Wmissing-prototypes] > drivers/scsi/be2iscsi/be_main.c:97:1: warning: no previous prototype for > 'beiscsi_log_enable_store' [-Wmissing-prototypes] > drivers/scsi/be2iscsi/be_main.c:116:1: warning: no previous prototype for > 'beiscsi_log_enable_init' [-Wmissing-prototypes] > drivers/scsi/be2iscsi/be_main.c:4587:5: warning: no previous prototype for > 'beiscsi_iotask_v2' [-Wmissing-prototypes] > drivers/scsi/be2iscsi/be_main.c:4976:6: warning: no previous prototype for > 'beiscsi_hba_attrs_init' [-Wmissing-prototypes] > > In fact, these functions are only used in the file in which they are declared and > don't need a declaration, but can be made static. > > So this patch marks these functions with 'static'. > > Signed-off-by: Baoyou Xie > --- [JB] Looks good. Reviewed by: Jitendra Bhivare
RE: [PATCH] be2iscsi: Use a more current logging style
> -Original Message-
> From: Joe Perches [mailto:j...@perches.com]
> Sent: Tuesday, August 16, 2016 3:57 PM
> To: Jitendra Bhivare; Christophe JAILLET; Jayamohan Kallickal; Ketan
Mukadam
> Cc: Bart Van Assche; James E.J. Bottomley; Martin K. Petersen; linux-
> s...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] be2iscsi: Use a more current logging style
>
> On Tue, 2016-08-16 at 11:32 +0530, Jitendra Bhivare wrote:
> > Thanks Joe for taking this up. It has been pending for long time from
> > our side.
>
> Thanks, not a problem, it took ~10 minutes.
>
> There was a bit of an issue about your reply though.
>
> First there was ~50 k of quoted stuff without any content
>
> > [ hundreds and hundreds of quoted lines ]
>
> and then this happened:
>
> > > diff --git a/drivers/scsi/be2iscsi/be_main.h
> > b/drivers/scsi/be2iscsi/be_main.h
> > >
> > > index aa9c682..7cce6e3 100644
> > > --- a/drivers/scsi/be2iscsi/be_main.h
> > > +++ b/drivers/scsi/be2iscsi/be_main.h
> > > @@ -1081,15 +1081,19 @@ struct hwi_context_memory {
> > > #define BEISCSI_LOG_CONFIG 0x0020 /* CONFIG Code Path */
> > > #define BEISCSI_LOG_ISCSI0x0040 /* SCSI/iSCSI Protocol
related
> > Logs */
> > >
> > >
> > > -#define __beiscsi_log(phba, level, fmt, ...)
\
> > > - shost_printk(level, phba->shost, fmt, ##__VA_ARGS__)
> > > -
> > > -#define beiscsi_log(phba, level, mask, prefix, fmt, ...) \
> > > +#define beiscsi_printk(level, phba, mask, fmt, ...)
\
> > > do {
> \
> > > - uint32_t log_value = phba->attr_log_enable; \
> > > - if (((mask) & log_value) || (level[1] <= '3')) \
> > > - __beiscsi_log(phba, level, prefix "_%d: " fmt, \
> > > - __LINE__, ##__VA_ARGS__); \
> > > + if ((mask) & (phba)->attr_log_enable) \
> > > + shost_printk(level, phba->shost,\
> > [JB] PCI dev_printk would be more useful with SCSI host_no included by
> > default in the message.
>
> This is a good note that seems simple enough, but I almost missed this.
>
> Given the reply at the top and the _very_ long uncommented quoted block,
I just
> about assumed it was a useless block quote that you didn't bother to
trim.
>
> Please make it easier to find your replies and notes by deleting
irrelevant quoted
> stuff.
>
> Also, I think I misread the code.
>
> The original code is <= '3' i.e.: show all KERN_ERR.
> That is not correct in the new code.
>
> I don't know the code well and don't have a test bed with the hardware.
>
> Is it possible for a beiscsi_ message to be called before
phba->pcidev is
> set to a valid value in beiscsi_hba_alloc? It appears the code is
careful to only
> use dev_ logging calls before probe.
[JB] KERN_ERR messages need to be logged irrespective of the masks.
I understand, that in some places, mask is unnecessarily passed.
I had made sure to call __beiscsi_log in some places.
Can we please keep it that way? So beiscsi_err calls dev_err directly or
is replaced with dev_err.
It's safe to assume pcidev will be valid for all beiscsi_log calls.
Will test your change on my setup before ack'ing.
Actually, we too wanted to get rid of BC_/BM_... line# way and replace
with
ABCD = error identifier.
A
B
CD
But that will be substantial change with some testing requirements. For
now, this looks good.
RE: [PATCH 1/2] be2iscsi: Fix error return code
> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of Christophe JAILLET
> Sent: Friday, August 12, 2016 3:33 PM
> To: jayamohan.kallic...@avagotech.com; j...@linux.vnet.ibm.com;
> ketan.muka...@avagotech.com; sony.j...@avagotech.com;
> martin.peter...@oracle.com
> Cc: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-
> janit...@vger.kernel.org; Christophe JAILLET
> Subject: [PATCH 1/2] be2iscsi: Fix error return code
>
> We know that 'ret' is not an error code because it has been tested a few
> lines
> above.
> So, if one of these function fails, 0 will be returned instead of an error
> code.
> Return -ENOMEM instead.
>
> Signed-off-by: Christophe JAILLET
> ---
> drivers/scsi/be2iscsi/be_main.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/be2iscsi/be_main.c
> b/drivers/scsi/be2iscsi/be_main.c
> index f05e7737107d..89ae6390b697 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -3286,8 +3286,10 @@ static int beiscsi_create_eqs(struct beiscsi_hba
> *phba,
> eq_vaddress = pci_alloc_consistent(phba->pcidev,
>num_eq_pages *
> PAGE_SIZE,
>&paddr);
> - if (!eq_vaddress)
> + if (!eq_vaddress) {
> + ret = -ENOMEM;
> goto create_eq_error;
> + }
>
> mem->va = eq_vaddress;
> ret = be_fill_queue(eq, phba->params.num_eq_entries, @@ -
> 3349,8 +3351,11 @@ static int beiscsi_create_cqs(struct beiscsi_hba *phba,
> cq_vaddress = pci_alloc_consistent(phba->pcidev,
>num_cq_pages *
> PAGE_SIZE,
>&paddr);
> - if (!cq_vaddress)
> + if (!cq_vaddress) {
> + ret = -ENOMEM;
> goto create_cq_error;
> + }
> +
> ret = be_fill_queue(cq, phba->params.num_cq_entries,
> sizeof(struct sol_cqe), cq_vaddress);
> if (ret) {
> @@ -5635,6 +5640,7 @@ static int beiscsi_dev_probe(struct pci_dev *pcidev,
> if (!phba) {
> dev_err(&pcidev->dev,
> "beiscsi_dev_probe - Failed in beiscsi_hba_alloc\n");
> + ret = -ENOMEM;
> goto disable_pci;
> }
>
> @@ -5754,6 +5760,7 @@ static int beiscsi_dev_probe(struct pci_dev *pcidev,
> beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
> "BM_%d : beiscsi_dev_probe-"
> "Failed to allocate work queue\n");
> + ret = -ENOMEM;
> goto free_twq;
> }
>
[JB] This still has to be fixed, right?
RE: [PATCH] be2iscsi: Use a more current logging style
Thanks Joe for taking this up. It has been pending for long time from our
side.
> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of Joe Perches
> Sent: Monday, August 15, 2016 12:26 AM
> To: Christophe JAILLET; Jayamohan Kallickal; Ketan Mukadam
> Cc: Bart Van Assche; James E.J. Bottomley; Martin K. Petersen; linux-
> s...@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH] be2iscsi: Use a more current logging style
>
> shost_printk macros are converted to beiscsi_ for a
> more current logging style.
>
> Consolidate the masked tests into a beiscsi_printk macro and
> use that to call shost_printk.
>
> Convert the __beiscsi_log macros to use shost_print directly.
>
> Miscellanea:
>
> o Remove the file prefix identifier and use kbasename(__FILE__)
> and stringify(__LINE__) to reduce code size in beiscsi_printk
> o Realign arguments
>
> Signed-off-by: Joe Perches
> ---
> drivers/scsi/be2iscsi/be_cmds.c | 142
> drivers/scsi/be2iscsi/be_iscsi.c | 223 ++--
> drivers/scsi/be2iscsi/be_main.c | 733
+++
> drivers/scsi/be2iscsi/be_main.h | 20 +-
> drivers/scsi/be2iscsi/be_mgmt.c | 270 +++---
> 5 files changed, 668 insertions(+), 720 deletions(-)
>
> diff --git a/drivers/scsi/be2iscsi/be_cmds.c
b/drivers/scsi/be2iscsi/be_cmds.c
> index a51bc0e..574d2f4 100644
> --- a/drivers/scsi/be2iscsi/be_cmds.c
> +++ b/drivers/scsi/be2iscsi/be_cmds.c
> @@ -95,8 +95,8 @@ int be_chk_reset_complete(struct beiscsi_hba *phba)
> }
>
> if ((status & 0x8000) || (!num_loop)) {
> - beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
> - "BC", "Failed in be_chk_reset_complete status
=
> 0x%x\n",
> + beiscsi_err(phba, BEISCSI_LOG_INIT,
> + "Failed in be_chk_reset_complete status =
0x%x\n",
> status);
> return -EIO;
> }
> @@ -135,9 +135,9 @@ struct be_mcc_wrb *alloc_mcc_wrb(struct beiscsi_hba
> *phba,
>
> spin_lock_bh(&phba->ctrl.mcc_lock);
> if (mccq->used == mccq->len) {
> - beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT |
> + beiscsi_err(phba, BEISCSI_LOG_INIT |
> BEISCSI_LOG_CONFIG | BEISCSI_LOG_MBOX,
> - "BC", "MCC queue full: WRB used %u tag avail
%u\n",
> + "MCC queue full: WRB used %u tag avail %u\n",
> mccq->used, phba->ctrl.mcc_tag_available);
> goto alloc_failed;
> }
> @@ -147,9 +147,9 @@ struct be_mcc_wrb *alloc_mcc_wrb(struct beiscsi_hba
> *phba,
>
> tag = phba->ctrl.mcc_tag[phba->ctrl.mcc_alloc_index];
> if (!tag) {
> - beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT |
> + beiscsi_err(phba, BEISCSI_LOG_INIT |
> BEISCSI_LOG_CONFIG | BEISCSI_LOG_MBOX,
> - "BC", "MCC tag 0 allocated: tag avail %u alloc
index
> %u\n",
> + "MCC tag 0 allocated: tag avail %u alloc index
%u\n",
> phba->ctrl.mcc_tag_available,
> phba->ctrl.mcc_alloc_index);
> goto alloc_failed;
> @@ -263,10 +263,10 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba
> *phba,
> set_bit(MCC_TAG_STATE_TIMEOUT,
> &phba->ctrl.ptag_state[tag].tag_state);
>
> - beiscsi_log(phba, KERN_ERR,
> + beiscsi_err(phba,
> BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
> BEISCSI_LOG_CONFIG,
> - "BC", "MBX Cmd Completion timed out\n");
> + "MBX Cmd Completion timed out\n");
> return -EBUSY;
> }
>
> @@ -289,22 +289,22 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba
> *phba,
> }
>
> if (status || addl_status) {
> - beiscsi_log(phba, KERN_WARNING,
> - BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
> - BEISCSI_LOG_CONFIG,
> - "BC", "MBX Cmd Failed for Subsys : %d Opcode :
%d
> with Status : %d and Extd_Status : %d\n",
> - mbx_hdr->subsystem,
> - mbx_hdr->opcode,
> - status, addl_status);
> + beiscsi_warn(phba,
> + BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
> + BEISCSI_LOG_CONFIG,
> + "MBX Cmd Failed for Subsys : %d Opcode : %d
with
> Status : %d and Extd_Status : %d\n",
> + mbx_hdr->subsystem,
> + mbx_hdr->opcode,
> + status, addl_status);
> rc = -EIO;
> if (status == MCC_STATUS_INSUFFICIENT_BUFFER) {
> mbx_resp_hdr = (struct be_cmd_resp_hdr *) mbx_hdr;
> -
