On Mon, 2014-06-09 at 14:02 -0700, Joe Perches wrote:
> On Mon, 2014-06-09 at 13:35 -0700, Jeff Kirsher wrote:
> > On Mon, 2014-06-09 at 17:21 +0400, Sergei Shtylyov wrote:
> > > On 06/09/2014 12:49 PM, Jeff Kirsher wrote:
> > > > From: Shannon Nelson <shannon.nelson@intel.com>
> 
> > > > If the Firmware sets these bits, it will trigger an AdminQ
> > > > interrupt to get the driver's attention to process the ARQ, which will
> > > > likely be enough to clear the actual issue.
>  
> > >     Hm, why not dev_err() here and below?
> 
> > The thought was that these should be more of "FYI..." type of messages
> > not "Oh Crap!..." messages.  So that is why dev_err() was not used,
> > although we are not opposed to changing it if you feel it warrants it in
> > a follow-up patch.
> []
> > > > +	if (val & I40E_PF_ATQLEN_ATQCRIT_MASK) {
> > > > +		dev_info(&pf->pdev->dev, "ASQ Critical Error detected\n");
> > > > +		val &= ~I40E_PF_ATQLEN_ATQCRIT_MASK;
> > > > +	}
> 
> I thought it was odd to have a "critical error"
> emitted at KERN_INFO
> 
> Maybe adding something like
> 	"ARQ should fix this automatically"
> would be enough.
> 
> Should any/all of these be ratelimited or maybe even
> changed to dev_dbg?
> 
> 

Well, as Shannon noted in the patch description:
"If the Firmware sets these bits, it will trigger an AdminQ
interrupt to get the driver's attention to process the ARQ, which will
likely be enough to clear the actual issue."

To me that means the log messages should not be filling up with these
types of messages and if they do occur, it should be transient and
resolve itself.  So does that warrant dev_dbg?