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

[Jitendra Bhivare <jitendra.bhivare@broadcom.com>]

> -----Original Message-----
> From: Joe Perches [mailto:joe@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-
> scsi@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_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_<level> 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_<level> 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 <category>
B <subcategory>
CD <error code>

But that will be substantial change with some testing requirements. For
now, this looks good.