Re: [PATCH 04/40] libata: move ata_{port,link,dev}_dbg to standard dev_XXX() macros

[Hannes Reinecke <hare@suse.de>]

On 3/25/20 4:45 PM, Bartlomiej Zolnierkiewicz wrote:
> 
> 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@samsung.com> 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@suse.de>
>>>>> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>>>>
>>>> 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!
>>
Well, the space can (should?) be fixed with a simple patch to 
__dev_printk() but just checking if 'dev_driver_string()' returns a 
value or not:

diff --git a/drivers/base/core.c b/drivers/base/core.c
index dbb0f9130f42..a84f170a1a1f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3635,10 +3635,15 @@ 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) {
+               if (dev_string_string(dev))
+                       dev_printk_emit(level[1] - '0', dev, "%s %s: %pV",
+                                       dev_driver_string(dev), 
dev_name(dev),
+                                       vaf);
+               else
+                       dev_printk_emit(level[1] - '0', dev, "%s: %pV",
+                                       dev_name(dev), vaf);
+       } else
                 printk("%s(NULL device *): %pV", level, vaf);
  }

which actually makes sense on its own.
As for the name change, yes, I'm not that happy with it, either.

I'll see if we can do some transport class magic such that the
names do not change.

(And no-one is to mention 'cake' and 'eat it' ...)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer