Re: [PATCH 1/2] libata: rework sysfs naming

[Hannes Reinecke <hare@suse.de>]

On 3/25/22 04:05, Damien Le Moal wrote:
> On 3/24/22 21:32, Hannes Reinecke wrote:
>> This patch adds a new dummy bus 'ata', which collects all ata device
>> objects like ata_port, ata_link, and ata_dev, and adds an 'ata' prefix
>> to the message log.
>> To be consistent with the other libata objects the 'ata_port' object name
>> has been changed from 'ata' to 'port'.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/ata/libata-transport.c | 21 +++++++++++--
>>   include/linux/libata.h         | 54 ++++++++++------------------------
>>   2 files changed, 34 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/ata/libata-transport.c 
>> b/drivers/ata/libata-transport.c
>> index ca129854a88c..555fe6e2293d 100644
>> --- a/drivers/ata/libata-transport.c
>> +++ b/drivers/ata/libata-transport.c
>> @@ -81,10 +81,13 @@ struct ata_internal {
>>       tdev_to_port((dev)->parent)
>> -/* Device objects are always created whit link objects */
>> +/* Device objects are always created with link objects */
>>   static int ata_tdev_add(struct ata_device *dev);
>>   static void ata_tdev_delete(struct ata_device *dev);
>> +struct bus_type ata_bus_type = {
>> +        .name        = "ata",
>> +};
>>   /*
>>    * Hack to allow attributes of the same name in different objects.
>> @@ -288,7 +291,9 @@ int ata_tport_add(struct device *parent,
>>       dev->parent = parent;
>>       ata_host_get(ap->host);
>>       dev->release = ata_tport_release;
>> -    dev_set_name(dev, "ata%d", ap->print_id);
>> +    dev->bus = &ata_bus_type;
>> +    dev_set_name(dev, "port%d", ap->print_id);
>> +
>>       transport_setup_device(dev);
>>       ata_acpi_bind_port(ap);
>>       error = device_add(dev);
>> @@ -444,6 +449,8 @@ int ata_tlink_add(struct ata_link *link)
>>       device_initialize(dev);
>>       dev->parent = &ap->tdev;
>>       dev->release = ata_tlink_release;
>> +    dev->bus = &ata_bus_type;
>> +
>>       if (ata_is_host_link(link))
>>           dev_set_name(dev, "link%d", ap->print_id);
>>       else
>> @@ -695,8 +702,10 @@ static int ata_tdev_add(struct ata_device *ata_dev)
>>       device_initialize(dev);
>>       dev->parent = &link->tdev;
>>       dev->release = ata_tdev_release;
>> +    dev->bus = &ata_bus_type;
>> +
>>       if (ata_is_host_link(link))
>> -        dev_set_name(dev, "dev%d.%d", ap->print_id,ata_dev->devno);
>> +        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);
>> @@ -822,8 +831,13 @@ __init int libata_transport_init(void)
>>       error = transport_class_register(&ata_dev_class);
>>       if (error)
>>           goto out_unregister_port;
>> +    error = bus_register(&ata_bus_type);
>> +    if (error)
>> +        goto out_unregister_bus;
> 
> Why is it needed to call bus_unregister() if bus_register() fails ? 
> Shouldn't this be a "goto out_unregister_dev" which does a 
> "transport_class_unregister(&ata_dev_class)" ?
> 
Ah yes. You are right.

>>       return 0;
>> + out_unregister_bus:
>> +    bus_unregister(&ata_bus_type);
>>    out_unregister_port:
>>       transport_class_unregister(&ata_port_class);
>>    out_unregister_link:
>> @@ -835,6 +849,7 @@ __init int libata_transport_init(void)
>>   void __exit libata_transport_exit(void)
>>   {
>> +    bus_unregister(&ata_bus_type);
>>       transport_class_unregister(&ata_link_class);
>>       transport_class_unregister(&ata_port_class);
>>       transport_class_unregister(&ata_dev_class);
>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index 0619ae462ecd..b17683b00c90 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -835,6 +835,7 @@ struct ata_port {
>>       struct ata_host        *host;
>>       struct device         *dev;
>>       struct device        tdev;
>> +    struct device        cdev;
> 
> This one is not used...
> 

Yeah, left-over from previous iteration.
I'll be resending.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
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