Borislav Petkov wrote:
On Fri, Jun 30, 2006 at 02:03:11AM +0900, Tejun Heo wrote:
<snip>
Hello,
I think this patchset is looking good generally. Please consider the
following.
* Don't make currently visible-by-default messages invisible-by-default
or vice-versa.
The way I see it, we print by default everything marked ERR/WARN/DRV, as it is
being done down in libata-core.c
* Don't mix visiable-by-default messages with debug messages. e.g. It
seems you made ATA_MSG_INFO debug category and put EH messages and some
debug messages into it. This is not good. EH messages should be
visible to user by default && enabling it shouldn't turn on debug
messages with it.
Well, it is sometimes unclear what level exactly a message should be but afterwe
have converted everything changing the level is as trivial as changing the
ATA_MSG_* arg passed to ata_(port/dev)_printk so this won't be a problem. An
initial overhaul of the levels will be needed once the conversion is done.
Also, I tried to stick to the levels that were there previously so since their
semantics changes too, now, that also introduces some small differences.
EH messages shouldn't be suppressed by default.
* During EH, IOs will stop and as a result the machine will act abnormal
and can stutter for tens of seconds.
* EH means something is wrong with the hardware or the driver, so we
need the log to analyze the problem.
* Initial probing is also done via EH.
I don't really see the benefit of discerning between DRV and INFO.
IMHO, DRV should just be INFO which is enabled by default.
* Please create ATA_MSG_DEBUG category and put
important-but-not-too-frequent debug messages into it.
Can you please define those "important-but-not-too-frequent" more precisely and
support it with an example in the code?
I've commented such cases w/ 'DEBUG' in the review of the patches.
Basically, any debug messages which isn't too frequent. I don't think
we need to go super-fine-grained with debug messages. CMD/SG/TRACE
generate way too many messages so they need to be in separate categories
but other than that I don't see much point in making debug messages
fine-grained.
* To me, DRV/INFO distinction doesn't seem to be clear.
INFO was revalidation messages, EH progress and DRV are standard driver
messages.
Hmm... I don't know. EH messages should be printed, so they need to go
into DRV. Then we have only revalidation messages left in INFO which
are few, so they can be put into DEBUG. So, I don't see why INFO and
DRV should be separate. Just rename DRV to INFO and put all standard
messages there.
Thanks.
--
tejun
-
: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
