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

[Hannes Reinecke <hare@xxxxxxx>]

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@xxxxxxx>
> > ---
> >   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@xxxxxxx                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer