On 3/25/20 4:34 PM, Geert Uytterhoeven wrote: > Hi Bartl, > > On Wed, Mar 25, 2020 at 3:56 PM Bartlomiej Zolnierkiewicz > <b.zolnierkie@xxxxxxxxxxx> wrote: >> On 3/24/20 2:26 PM, Geert Uytterhoeven wrote: >>> 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). > > True. > >> 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()). > > Thanks for the suggestion! > >> --- 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 : ""))); > > Aren't "dev->class->name" and "(&dev->tclass->class)->name" > exactly the same, as the first member of struct transport_class is > a struct class object? struct class is embedded in struct transport_class but it doesn't have to be the same class as pointed by dev->class (for ATA dev->class should be NULL). > (If they're not the same, and I need more coffee, I'm still afraid this is > too fragile. Then there is no guarantee the pointer points to a real > transport_class object, so this might cause a crash.) I see, we can't do unnamed union optimization in <linux/device.h> but otherwise it should be OK, no? >> --- 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; > > Can't you just do dev->class = &ata_port_class.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; > > ... and dev->class = &ata_link_class.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; > > ... and dev->class = &ata_dev_class.class... > >> --- 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); > > ... and drop all changes to device.h? That would have unintended consequences that we don't want (driver core code checks dev->class existence not only in dev_driver_string()).. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics