On 5/26/24 3:39 PM, Krzysztof Olędzki wrote:
> Thank you. How about the following patch? I'm sure I missed something
> but it should bring us close to where we were before the logging cleanup.
>
> If something like this is acceptable, perpahs it would be also possible
> to include it in the -stable kernels, I think up to and including 5.15?
Please send a proper patch with afixes tag and cc-stable tag instead of
something as a reply to this thread. We will do the review of that.
>
> From b1f93347828a3d8c52ae4a73f9fb48341d0c2afb Mon Sep 17 00:00:00 2001
> From: Krzysztof Piotr Oledzki <ole@xxxxxx>
> Date: Sat, 25 May 2024 22:35:58 -0700
> Subject: [PATCH] ata: libata: restore visibility of important messages
>
> With the recent cleanup and migration to standard
> pr_{debug,info,notice,warn,err} macros instead of the
> hand-crafted printk helpers some important messages
> disappeared from dmesg unless dynamic debug is enabled.
Very short lines. Please format this up to 72 chars per line.
And instead of using the very subjective "recent" term for the message changes,
please reference the commit that did it.
>
> Restore their visibility by promoting them to info or err (when
> appropriate).
>
> Also, improve or add information about features disabled due to
> ATA_HORKAGE workarounds like NONCQ, NO_NCQ_ON_ATI and NO_FUA.
This part should be a different patch as it is not "fixing" the previous
changes but adds new messages.
>
> For ATA_HORKAGE_DIAGNOSTIC and ATA_HORKAGE_FIRMWARE_WARN convert the
> message to a single line and update the ata.wiki.kernel.org link.
Hmmm... I have no idea who created that wiki nor if it is maintained at all...
We (the libata maintainers) are for sure not maintaining it. So I would prefer
to entirely remove that link as I am not sure the information there is correct
at all. If anything needs to be documented, then add a note in
Documentation/./driver-api/libata.rst". (note that I have actually never looked
at this doc file either since taking on libata maintainership. Its content may
not be up-to-date. Will need to review it.).
> Signed-off-by: Krzysztof Piotr Oledzki <ole@xxxxxx>
> ---
> drivers/ata/libata-core.c | 49 +++++++++++++++++++++------------------
> drivers/ata/libata-eh.c | 2 +-
> 2 files changed, 28 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 4f35aab81a0a..0603849692ae 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1793,7 +1793,7 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
>
> if (err_mask) {
> if (err_mask & AC_ERR_NODEV_HINT) {
> - ata_dev_dbg(dev, "NODEV after polling detection\n");
> + ata_dev_err(dev, "NODEV after polling detection\n");
The "NODEV" mention here makes no sense given that we return -ENOENT. Let's
instead print the error mask with the usual format.
> return -ENOENT;
> }
>
> @@ -1825,8 +1825,8 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
> * both flavors of IDENTIFYs which happens
> * sometimes with phantom devices.
> */
> - ata_dev_dbg(dev,
> - "both IDENTIFYs aborted, assuming NODEV\n");
> + ata_dev_info(dev,
> + "both IDENTIFYs aborted, assuming NODEV\n");
This is not really normal, so let's make this a "warn". And replace "NODEV"
with "no device present.".
> return -ENOENT;
> }
>
> @@ -1857,9 +1857,9 @@ int ata_dev_read_id(struct ata_device *dev, unsigned int *p_class,
> if (class == ATA_DEV_ATA || class == ATA_DEV_ZAC) {
> if (!ata_id_is_ata(id) && !ata_id_is_cfa(id))
> goto err_out;
> - if (ap->host->flags & ATA_HOST_IGNORE_ATA &&
> - ata_id_is_ata(id)) {
> - ata_dev_dbg(dev,
> + if ((ap->host->flags & ATA_HOST_IGNORE_ATA) &&
> + ata_id_is_ata(id)) {
> + ata_dev_info(dev,
> "host indicates ignore ATA devices, ignored\n");
Let's improve the wording here as "host" has a meaning (e.g. scsi host). Let's
make this something like:
ata_dev_info(dev,
"ATA_HOST_IGNORE_ATA set: ignoring device.\n");
> return -ENOENT;
> }
> @@ -2247,7 +2247,8 @@ static void ata_dev_config_ncq_send_recv(struct ata_device *dev)
> memcpy(cmds, ap->sector_buf, ATA_LOG_NCQ_SEND_RECV_SIZE);
>
> if (dev->horkage & ATA_HORKAGE_NO_NCQ_TRIM) {
> - ata_dev_dbg(dev, "disabling queued TRIM support\n");
> + ata_dev_info(dev, "disabling queued TRIM - "
> + "known buggy device\n");
There is no need to change the message text here: it is clear. And because this
is a horkage, let's use ata_dev_warn() and capitalize the first letter of the
message:
ata_dev_warn(dev, "Disabling NCQ TRIM support\n");
Note that we could define a common format for all horkage related messages. E.g.:
ata_dev_warn(dev,
"ATA_HORKAGE_NO_NCQ_TRIM set: Disabling queued TRIM support\n");
But I am not sure that is really useful as long as the same message text is not
repeated for a different reason. That will avoid the long lines :)
> cmds[ATA_LOG_NCQ_SEND_RECV_DSM_OFFSET] &=
> ~ATA_LOG_NCQ_SEND_RECV_DSM_TRIM;
> }
> @@ -2335,13 +2336,13 @@ static int ata_dev_config_ncq(struct ata_device *dev,
> if (!IS_ENABLED(CONFIG_SATA_HOST))
> return 0;
> if (dev->horkage & ATA_HORKAGE_NONCQ) {
> - snprintf(desc, desc_sz, "NCQ (not used)");
> + snprintf(desc, desc_sz, "NCQ (not used - known buggy device)");
I think we should replace this snprintf() with a ata_dev_warn() message similar
to the above NCQ TRIM. E.g.:
ata_dev_warn(dev, "Disabling NCQ support\n");
> return 0;
> }
>
> if (dev->horkage & ATA_HORKAGE_NO_NCQ_ON_ATI &&
> ata_dev_check_adapter(dev, PCI_VENDOR_ID_ATI)) {
> - snprintf(desc, desc_sz, "NCQ (not used)");
> + snprintf(desc, desc_sz, "NCQ (not used - known buggy device/host adapter)");
Same as above.
> return 0;
> }
>
> @@ -2397,7 +2398,7 @@ static void ata_dev_config_sense_reporting(struct ata_device *dev)
>
> err_mask = ata_dev_set_feature(dev, SETFEATURE_SENSE_DATA, 0x1);
> if (err_mask) {
> - ata_dev_dbg(dev,
> + ata_dev_err(dev,
> "failed to enable Sense Data Reporting, Emask 0x%x\n",
> err_mask);
> }
> @@ -2479,7 +2480,7 @@ static void ata_dev_config_trusted(struct ata_device *dev)
>
> trusted_cap = get_unaligned_le64(&ap->sector_buf[40]);
> if (!(trusted_cap & (1ULL << 63))) {
> - ata_dev_dbg(dev,
> + ata_dev_err(dev,
> "Trusted Computing capability qword not valid!\n");
I think this fits in one line. So let's drop the line break.
> return;
> }
> @@ -2688,9 +2689,15 @@ static void ata_dev_config_fua(struct ata_device *dev)
> if (!(dev->flags & ATA_DFLAG_LBA48) || !ata_id_has_fua(dev->id))
> goto nofua;
>
> - /* Ignore known bad devices and devices that lack NCQ support */
> - if (!ata_ncq_supported(dev) || (dev->horkage & ATA_HORKAGE_NO_FUA))
> + /* Ignore devices that lack NCQ support */
> + if (!ata_ncq_supported(dev))
> + goto nofua;
> +
> + /* Finally, ignore buggy devices */
> + if (dev->horkage & ATA_HORKAGE_NO_FUA) {
> + ata_dev_info(dev, "disabling FUA - known buggy device");
Use ata_dev_warn():
ata_dev_warn(dev, "Disabling FUA support.\n");
> goto nofua;
> + }
>
> dev->flags |= ATA_DFLAG_FUA;
>
> @@ -3060,24 +3067,22 @@ int ata_dev_configure(struct ata_device *dev)
> if (ap->ops->dev_config)
> ap->ops->dev_config(dev);
>
> - if (dev->horkage & ATA_HORKAGE_DIAGNOSTIC) {
> + if ((dev->horkage & ATA_HORKAGE_DIAGNOSTIC) && print_info) {
> /* Let the user know. We don't want to disallow opens for
> rescue purposes, or in case the vendor is just a blithering
> idiot. Do this after the dev_config call as some controllers
> with buggy firmware may want to avoid reporting false device
> bugs */
While at it, please fix this comment block format.
>
> - if (print_info) {
> - ata_dev_warn(dev,
> -"Drive reports diagnostics failure. This may indicate a drive\n");
> - ata_dev_warn(dev,
> -"fault or invalid emulation. Contact drive vendor for information.\n");
> - }
> + ata_dev_warn(dev, "Drive reports diagnostics failure."
> + " This may indicate a drive fault or invalid emulation."
> + " Contact drive vendor for information.\n");
> }
>
> if ((dev->horkage & ATA_HORKAGE_FIRMWARE_WARN) && print_info) {
> - ata_dev_warn(dev, "WARNING: device requires firmware update to be fully functional\n");
> - ata_dev_warn(dev, " contact the vendor or visit http://ata.wiki.kernel.org\n";);
> + ata_dev_warn(dev, "WARNING: device requires firmware update to be"
> + " fully functional contact the vendor or visit"
> + " http://ata.wiki.kernel.org/index.php/Known_issues\n";);
As mentioned, I think we shoud drop the wiki link. It is not maintained.
> }
>
> return 0;
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 214b935c2ced..5b9382ef261c 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -2667,7 +2667,7 @@ int ata_eh_reset(struct ata_link *link, int classify,
>
> if (rc) {
> if (rc == -ENOENT) {
> - ata_link_dbg(link, "port disabled--ignoring\n");
> + ata_link_info(link, "port disabled--ignoring\n");
I think ata_link_warn() is better here. And let's reformat the message:
ata_link_warn(link,
"Ignoring disabled port\n");
> ehc->i.action &= ~ATA_EH_RESET;
>
> ata_for_each_dev(dev, link, ALL)
--
Damien Le Moal
Western Digital Research
