On 3/24/20 2:26 PM, Geert Uytterhoeven wrote:
> Hi Hannes,
>
> On Tue, 3 Mar 2020, Hannes Reinecke wrote:
>> Use standard dev_{dbg,info,notice,warn,err} macros instead of the
>> hand-crafted printk helpers.
>>
>> Signed-off-by: Hannes Reinecke <hare@xxxxxxx>
>> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
>
> This is now commit ad9f23bd12e1d721 ("libata: move
> ata_{port,link,dev}_dbg to standard dev_XXX() macros") in
> linux-block/for-next.
>
> This patch causes an intriguing change in boot messages, adding a space
> at the beginning of each printed line:
>
> scsi host0: sata_rcar
> -ata1: SATA max UDMA/133 irq 117
> + ata1: SATA max UDMA/133 irq 117
>
> -ata1: link resume succeeded after 1 retries
> + link1: link resume succeeded after 1 retries
>
> -ata1: SATA link down (SStatus 0 SControl 300)
> + link1: SATA link down (SStatus 0 SControl 300)
>
> It turns out dev_driver_string(&link->tdev) returns an empty string, as
> its driver field is NULL, so __dev_printk() prints the empty string and
> the device name, separated by a space.
>
> At first I thought this was a bug in rcar-sata, lacking some setup that
> was harmless before, but it turns out other drivers (e.g. pata-falcon)
> show the same issue:
>
> scsi host0: pata_falcon
> -ata1: PATA max PIO4 cmd 0xfff00000 ctl 0xfff00039
> + ata1: PATA max PIO4 cmd 0xfff00000 ctl 0xfff00039
>
> -ata1.01: NODEV after polling detection
> -ata1.00: ATA-2: Sarge m68k, , max PIO2
> -ata1.00: 2118816 sectors, multi 0: LBA
> -ata1.00: configured for PIO
> + dev1.0: ATA-2: Sarge m68k, , max PIO2
> + dev1.0: 2118816 sectors, multi 0: LBA
> + dev1.0: configured for PIO
I'm more worried about change of ATA devices and ATA links names
(unfortunately we cannot change the ones used in libata-transport.c
as they are a part of ABI exposed through sysfs).
One way to improve the situation is to use transport classes names
for dev_printk() when no other means are available, please see/try
the patch below (compile tested only). It also includes fixup for
extra space issue (change to __dev_printk()).
We should probably consider reverting the commit if the approach
with using transport classes names is not feasible (we should not be
confusing users with names like "dev1.0" instead of "ata1.00" etc.,
sorry for not catching this earlier).
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
---
drivers/ata/libata-transport.c | 3 +++
drivers/base/core.c | 14 +++++++++-----
include/linux/device.h | 7 ++++++-
3 files changed, 18 insertions(+), 6 deletions(-)
Index: b/drivers/ata/libata-transport.c
===================================================================
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -289,6 +289,7 @@ int ata_tport_add(struct device *parent,
ata_host_get(ap->host);
dev->release = ata_tport_release;
dev_set_name(dev, "ata%d", ap->print_id);
+ dev->tclass = &ata_port_class;
transport_setup_device(dev);
ata_acpi_bind_port(ap);
error = device_add(dev);
@@ -418,6 +419,7 @@ int ata_tlink_add(struct ata_link *link)
dev_set_name(dev, "link%d", ap->print_id);
else
dev_set_name(dev, "link%d.%d", ap->print_id, link->pmp);
+ dev->tclass = &ata_link_class;
transport_setup_device(dev);
@@ -669,6 +671,7 @@ static int ata_tdev_add(struct ata_devic
dev_set_name(dev, "dev%d.%d", ap->print_id,ata_dev->devno);
else
dev_set_name(dev, "dev%d.%d.0", ap->print_id, link->pmp);
+ dev->tclass = &ata_dev_class;
transport_setup_device(dev);
ata_acpi_bind_dev(ata_dev);
Index: b/drivers/base/core.c
===================================================================
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1225,7 +1225,8 @@ const char *dev_driver_string(const stru
drv = READ_ONCE(dev->driver);
return drv ? drv->name :
(dev->bus ? dev->bus->name :
- (dev->class ? dev->class->name : ""));
+ (dev->class ? dev->class->name :
+ (dev->tclass ? (&dev->tclass->class)->name : "")));
}
EXPORT_SYMBOL(dev_driver_string);
@@ -3784,10 +3785,13 @@ EXPORT_SYMBOL(dev_printk_emit);
static void __dev_printk(const char *level, const struct device *dev,
struct va_format *vaf)
{
- if (dev)
- dev_printk_emit(level[1] - '0', dev, "%s %s: %pV",
- dev_driver_string(dev), dev_name(dev), vaf);
- else
+ if (dev) {
+ const char *drv_str = dev_driver_string(dev);
+
+ dev_printk_emit(level[1] - '0', dev, "%s%s%s: %pV",
+ drv_str, strcmp(drv_str, "") ? " " : "",
+ dev_name(dev), vaf);
+ } else
printk("%s(NULL device *): %pV", level, vaf);
}
Index: b/include/linux/device.h
===================================================================
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -29,6 +29,7 @@
#include <linux/device/bus.h>
#include <linux/device/class.h>
#include <linux/device/driver.h>
+#include <linux/transport_class.h>
#include <asm/device.h>
struct device;
@@ -608,7 +609,11 @@ struct device {
spinlock_t devres_lock;
struct list_head devres_head;
- struct class *class;
+ union {
+ struct class *class;
+ struct transport_class *tclass;
+ };
+
const struct attribute_group **groups; /* optional groups */
void (*release)(struct device *dev);
