libata: improve EH report formatting

Requiring LLDs to format multiple error description messages properly
doesn't work too well.  Help LLDs a bit by making ata_ehi_push_desc()
insert ", " on each invocation.  __ata_ehi_push_desc() is the raw
version without the automatic separator.

While at it, make ehi_desc interface proper functions instead of
macros.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Signed-off-by: Jeff Garzik <jeff@garzik.org>

7 files changed, 98 insertions, 35 deletions

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index c5034d450c62..210292cd8ad1 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1289,12 +1289,12 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
         if (irq_stat & PORT_IRQ_IF_ERR) {
                 err_mask |= AC_ERR_ATA_BUS;
                 action |= ATA_EH_SOFTRESET;
-                ata_ehi_push_desc(ehi, ", interface fatal error");
+                ata_ehi_push_desc(ehi, "interface fatal error");
         }
 
         if (irq_stat & (PORT_IRQ_CONNECT | PORT_IRQ_PHYRDY)) {
                 ata_ehi_hotplugged(ehi);
-                ata_ehi_push_desc(ehi, ", %s", irq_stat & PORT_IRQ_CONNECT ?
+                ata_ehi_push_desc(ehi, "%s", irq_stat & PORT_IRQ_CONNECT ?
                         "connection status changed" : "PHY RDY changed");
         }
 
@@ -1303,7 +1303,7 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
 
                 err_mask |= AC_ERR_HSM;
                 action |= ATA_EH_SOFTRESET;
-                ata_ehi_push_desc(ehi, ", unknown FIS %08x %08x %08x %08x",
+                ata_ehi_push_desc(ehi, "unknown FIS %08x %08x %08x %08x",
                                   unk[0], unk[1], unk[2], unk[3]);
         }
 
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 39a8e986a4ea..ecbc3278238a 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6945,6 +6945,9 @@ EXPORT_SYMBOL_GPL(ata_pci_default_filter);
 EXPORT_SYMBOL_GPL(ata_pci_clear_simplex);
 #endif /* CONFIG_PCI */
 
+EXPORT_SYMBOL_GPL(__ata_ehi_push_desc);
+EXPORT_SYMBOL_GPL(ata_ehi_push_desc);
+EXPORT_SYMBOL_GPL(ata_ehi_clear_desc);
 EXPORT_SYMBOL_GPL(ata_eng_timeout);
 EXPORT_SYMBOL_GPL(ata_port_schedule_eh);
 EXPORT_SYMBOL_GPL(ata_port_abort);
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 9aa62a0754f6..96b184ebf708 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -85,6 +85,71 @@ static void ata_eh_handle_port_resume(struct ata_port *ap)
 { }
 #endif /* CONFIG_PM */
 
+static void __ata_ehi_pushv_desc(struct ata_eh_info *ehi, const char *fmt,
+                                 va_list args)
+{
+        ehi->desc_len += vscnprintf(ehi->desc + ehi->desc_len,
+                                     ATA_EH_DESC_LEN - ehi->desc_len,
+                                     fmt, args);
+}
+
+/**
+ *      __ata_ehi_push_desc - push error description without adding separator
+ *      @ehi: target EHI
+ *      @fmt: printf format string
+ *
+ *      Format string according to @fmt and append it to @ehi->desc.
+ *
+ *      LOCKING:
+ *      spin_lock_irqsave(host lock)
+ */
+void __ata_ehi_push_desc(struct ata_eh_info *ehi, const char *fmt, ...)
+{
+        va_list args;
+
+        va_start(args, fmt);
+        __ata_ehi_pushv_desc(ehi, fmt, args);
+        va_end(args);
+}
+
+/**
+ *      ata_ehi_push_desc - push error description with separator
+ *      @ehi: target EHI
+ *      @fmt: printf format string
+ *
+ *      Format string according to @fmt and append it to @ehi->desc.
+ *      If @ehi->desc is not empty, ", " is added in-between.
+ *
+ *      LOCKING:
+ *      spin_lock_irqsave(host lock)
+ */
+void ata_ehi_push_desc(struct ata_eh_info *ehi, const char *fmt, ...)
+{
+        va_list args;
+
+        if (ehi->desc_len)
+                __ata_ehi_push_desc(ehi, ", ");
+
+        va_start(args, fmt);
+        __ata_ehi_pushv_desc(ehi, fmt, args);
+        va_end(args);
+}
+
+/**
+ *      ata_ehi_clear_desc - clean error description
+ *      @ehi: target EHI
+ *
+ *      Clear @ehi->desc.
+ *
+ *      LOCKING:
+ *      spin_lock_irqsave(host lock)
+ */
+void ata_ehi_clear_desc(struct ata_eh_info *ehi)
+{
+        ehi->desc[0] = '\0';
+        ehi->desc_len = 0;
+}
+
 static void ata_ering_record(struct ata_ering *ering, int is_io,
                              unsigned int err_mask)
 {
@@ -1524,14 +1589,14 @@ static void ata_eh_report(struct ata_port *ap)
                                ehc->i.err_mask, ap->sactive, ehc->i.serror,
                                ehc->i.action, frozen);
                 if (desc)
-                        ata_dev_printk(ehc->i.dev, KERN_ERR, "(%s)\n", desc);
+                        ata_dev_printk(ehc->i.dev, KERN_ERR, "%s\n", desc);
         } else {
                 ata_port_printk(ap, KERN_ERR, "exception Emask 0x%x "
                                 "SAct 0x%x SErr 0x%x action 0x%x%s\n",
                                 ehc->i.err_mask, ap->sactive, ehc->i.serror,
                                 ehc->i.action, frozen);
                 if (desc)
-                        ata_port_printk(ap, KERN_ERR, "(%s)\n", desc);
+                        ata_port_printk(ap, KERN_ERR, "%s\n", desc);
         }
 
         for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 80ade5b93b86..b4b737e081e8 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -1411,12 +1411,12 @@ static void mv_err_intr(struct ata_port *ap, struct ata_queued_cmd *qc)
                         EDMA_ERR_INTRL_PAR)) {
                 err_mask |= AC_ERR_ATA_BUS;
                 action |= ATA_EH_HARDRESET;
-                ata_ehi_push_desc(ehi, ", parity error");
+                ata_ehi_push_desc(ehi, "parity error");
         }
         if (edma_err_cause & (EDMA_ERR_DEV_DCON | EDMA_ERR_DEV_CON)) {
                 ata_ehi_hotplugged(ehi);
                 ata_ehi_push_desc(ehi, edma_err_cause & EDMA_ERR_DEV_DCON ?
-                        ", dev disconnect" : ", dev connect");
+                        "dev disconnect" : "dev connect");
         }
 
         if (IS_GEN_I(hpriv)) {
@@ -1425,7 +1425,7 @@ static void mv_err_intr(struct ata_port *ap, struct ata_queued_cmd *qc)
                 if (edma_err_cause & EDMA_ERR_SELF_DIS_5) {
                         struct mv_port_priv *pp = ap->private_data;
                         pp->pp_flags &= ~MV_PP_FLAG_EDMA_EN;
-                        ata_ehi_push_desc(ehi, ", EDMA self-disable");
+                        ata_ehi_push_desc(ehi, "EDMA self-disable");
                 }
         } else {
                 eh_freeze_mask = EDMA_EH_FREEZE;
@@ -1433,7 +1433,7 @@ static void mv_err_intr(struct ata_port *ap, struct ata_queued_cmd *qc)
                 if (edma_err_cause & EDMA_ERR_SELF_DIS) {
                         struct mv_port_priv *pp = ap->private_data;
                         pp->pp_flags &= ~MV_PP_FLAG_EDMA_EN;
-                        ata_ehi_push_desc(ehi, ", EDMA self-disable");
+                        ata_ehi_push_desc(ehi, "EDMA self-disable");
                 }
 
                 if (edma_err_cause & EDMA_ERR_SERR) {
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index db81e3efa5ec..5d943da042f7 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -715,19 +715,20 @@ static int nv_adma_check_cpb(struct ata_port *ap, int cpb_num, int force_err)
                 int freeze = 0;
 
                 ata_ehi_clear_desc(ehi);
-                ata_ehi_push_desc(ehi, "CPB resp_flags 0x%x", flags );
+                __ata_ehi_push_desc(ehi, "CPB resp_flags 0x%x: ", flags );
                 if (flags & NV_CPB_RESP_ATA_ERR) {
-                        ata_ehi_push_desc(ehi, ": ATA error");
+                        ata_ehi_push_desc(ehi, "ATA error");
                         ehi->err_mask |= AC_ERR_DEV;
                 } else if (flags & NV_CPB_RESP_CMD_ERR) {
-                        ata_ehi_push_desc(ehi, ": CMD error");
+                        ata_ehi_push_desc(ehi, "CMD error");
                         ehi->err_mask |= AC_ERR_DEV;
                 } else if (flags & NV_CPB_RESP_CPB_ERR) {
-                        ata_ehi_push_desc(ehi, ": CPB error");
+                        ata_ehi_push_desc(ehi, "CPB error");
                         ehi->err_mask |= AC_ERR_SYSTEM;
                         freeze = 1;
                 } else {
                         /* notifier error, but no error in CPB flags? */
+                        ata_ehi_push_desc(ehi, "unknown");
                         ehi->err_mask |= AC_ERR_OTHER;
                         freeze = 1;
                 }
@@ -854,20 +855,21 @@ static irqreturn_t nv_adma_interrupt(int irq, void *dev_instance)
                                 struct ata_eh_info *ehi = &ap->eh_info;
 
                                 ata_ehi_clear_desc(ehi);
-                                ata_ehi_push_desc(ehi, "ADMA status 0x%08x", status );
+                                __ata_ehi_push_desc(ehi, "ADMA status 0x%08x: ", status );
                                 if (status & NV_ADMA_STAT_TIMEOUT) {
                                         ehi->err_mask |= AC_ERR_SYSTEM;
-                                        ata_ehi_push_desc(ehi, ": timeout");
+                                        ata_ehi_push_desc(ehi, "timeout");
                                 } else if (status & NV_ADMA_STAT_HOTPLUG) {
                                         ata_ehi_hotplugged(ehi);
-                                        ata_ehi_push_desc(ehi, ": hotplug");
+                                        ata_ehi_push_desc(ehi, "hotplug");
                                 } else if (status & NV_ADMA_STAT_HOTUNPLUG) {
                                         ata_ehi_hotplugged(ehi);
-                                        ata_ehi_push_desc(ehi, ": hot unplug");
+                                        ata_ehi_push_desc(ehi, "hot unplug");
                                 } else if (status & NV_ADMA_STAT_SERROR) {
                                         /* let libata analyze SError and figure out the cause */
-                                        ata_ehi_push_desc(ehi, ": SError");
-                                }
+                                        ata_ehi_push_desc(ehi, "SError");
+                                } else
+                                        ata_ehi_push_desc(ehi, "unknown");
                                 ata_port_freeze(ap);
                                 continue;
                         }
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index e538edc1b4ea..e201f1cab66d 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -814,16 +814,16 @@ static void sil24_error_intr(struct ata_port *ap)
 
         if (irq_stat & (PORT_IRQ_PHYRDY_CHG | PORT_IRQ_DEV_XCHG)) {
                 ata_ehi_hotplugged(ehi);
-                ata_ehi_push_desc(ehi, ", %s",
-                               irq_stat & PORT_IRQ_PHYRDY_CHG ?
-                               "PHY RDY changed" : "device exchanged");
+                ata_ehi_push_desc(ehi, "%s",
+                                  irq_stat & PORT_IRQ_PHYRDY_CHG ?
+                                  "PHY RDY changed" : "device exchanged");
                 freeze = 1;
         }
 
         if (irq_stat & PORT_IRQ_UNK_FIS) {
                 ehi->err_mask |= AC_ERR_HSM;
                 ehi->action |= ATA_EH_SOFTRESET;
-                ata_ehi_push_desc(ehi , ", unknown FIS");
+                ata_ehi_push_desc(ehi, "unknown FIS");
                 freeze = 1;
         }
 
@@ -842,11 +842,11 @@ static void sil24_error_intr(struct ata_port *ap)
                 if (ci && ci->desc) {
                         err_mask |= ci->err_mask;
                         action |= ci->action;
-                        ata_ehi_push_desc(ehi, ", %s", ci->desc);
+                        ata_ehi_push_desc(ehi, "%s", ci->desc);
                 } else {
                         err_mask |= AC_ERR_OTHER;
                         action |= ATA_EH_SOFTRESET;
-                        ata_ehi_push_desc(ehi, ", unknown command error %d",
+                        ata_ehi_push_desc(ehi, "unknown command error %d",
                                           cerr);
                 }
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 5d3df6cde272..94b37d180680 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -910,16 +910,9 @@ extern void ata_do_eh(struct ata_port *ap, ata_prereset_fn_t prereset,
 /*
  * ata_eh_info helpers
  */
-#define ata_ehi_push_desc(ehi, fmt, args...) do { \
-        (ehi)->desc_len += scnprintf((ehi)->desc + (ehi)->desc_len, \
-                                     ATA_EH_DESC_LEN - (ehi)->desc_len, \
-                                     fmt , ##args); \
-} while (0)
-
-#define ata_ehi_clear_desc(ehi) do { \
-        (ehi)->desc[0] = '\0'; \
-        (ehi)->desc_len = 0; \
-} while (0)
+extern void __ata_ehi_push_desc(struct ata_eh_info *ehi, const char *fmt, ...);
+extern void ata_ehi_push_desc(struct ata_eh_info *ehi, const char *fmt, ...);
+extern void ata_ehi_clear_desc(struct ata_eh_info *ehi);
 
 static inline void __ata_ehi_hotplugged(struct ata_eh_info *ehi)
 {