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

[Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>]

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!
> 
>> --- 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