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:joe@xxxxxxxxxxx]
> > 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@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > 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.