Re: [PATCH] be2iscsi: Use a more current logging style

[Joe Perches]

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

[Jitendra Bhivare]

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

[Joe Perches]

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

[Jitendra Bhivare]

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