Re: [PATCH] be2iscsi: Use a more current logging style
On Wed, 2016-08-17 at 09:20 +0530, Jitendra Bhivare wrote: > > > > -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-ker...@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_ISCSI 0x0040 /* 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. I did as well. > Can we please keep it that way? So beiscsi_err calls dev_err directly or > is replaced with dev_err. No worries, I'll respin the series after Christophe's patches are applied. > It's safe to assume pcidev will be valid for all beiscsi_log calls. > Will test your change on my setup before ack'ing. Don't bother until you get another patchset. I suggest you fix your email client when sending replies to me and to lkml. What I received is very difficult to read due to the odd line wrapping. -- 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] 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-ker...@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. -- 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] 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_ISCSI 0x0040 /* 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. -- 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] 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-ker...@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 <j...@perches.com> > --- > 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(>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, > >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, > + BEI
[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(>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, >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; - beiscsi_log(phba, KERN_WARNING, - BEISCSI_LOG_INIT | BEISCSI_LOG_EH | - BEISCSI_LOG_CONFIG, - "BC", "Insufficient Buffer Error Resp_Len : %d Actual_Resp_Len : %d\n", - mbx_resp_hdr->response_length, - mbx_resp_hdr->actual_resp_len); + beiscsi_warn(phba, +BEISCSI_LOG_INIT | BEISCSI_LOG_EH | +