| Message ID | 20220324123204.61534-3-hare@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | libata: sysfs naming | expand |
On 3/24/22 21:32, Hannes Reinecke wrote: > Add a config option 'ATA_SYSFS_COMPAT' to create a compability s/compability/compatibility > 'ata' symlink in the PCI device sysfs directory. I am not yet convinced if this new config option is really necessary... We could create the symlink unconditionally, no ? In any case, I would like to at least reduce the number of #ifdef. So what about something like this on top of your patch: diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c index a66c3480bdcf..fa249638bfb6 100644 --- a/drivers/ata/libata-transport.c +++ b/drivers/ata/libata-transport.c @@ -251,6 +251,40 @@ static int ata_tport_match(struct attribute_container *cont, return &ata_scsi_transport_template->host_attrs.ac == cont; } +#ifdef CONFIG_ATA_SYSFS_COMPAT + +static int ata_tport_compat_link_add(struct ata_port *ap) +{ + struct device *dev = &ap->tdev; + struct device *parent = dev->parent; + char compat_name[64]; + + sprintf(compat_name, "ata%d", ap->print_id); + + return sysfs_create_link(&parent->kobj, &dev->kobj, compat_name); +} + +static void ata_tport_compat_link_delete(struct ata_port *ap) +{ + struct device *dev = &ap->tdev; + struct device *parent = dev->parent; + char compat_name[64]; + + sprintf(compat_name, "ata%d", ap->print_id); + sysfs_remove_link(&parent->kobj, compat_name); +} + +#else + +static inline int ata_tport_compat_link_add(struct ata_port *ap) +{ + return 0; +} + +static inline void ata_tport_compat_link_delete(struct ata_port *ap) {} + +#endif + /** * ata_tport_delete -- remove ATA PORT * @ap: ATA PORT to remove @@ -260,13 +294,8 @@ static int ata_tport_match(struct attribute_container *cont, void ata_tport_delete(struct ata_port *ap) { struct device *dev = &ap->tdev; -#ifdef CONFIG_ATA_SYSFS_COMPAT - struct device *parent = dev->parent; - char compat_name[64]; - sprintf(compat_name, "ata%d", ap->print_id); - sysfs_remove_link(&parent->kobj, compat_name); -#endif + ata_tport_compat_link_delete(ap); ata_tlink_delete(&ap->link); transport_remove_device(dev); @@ -290,9 +319,6 @@ int ata_tport_add(struct device *parent, { int error; struct device *dev = &ap->tdev; -#ifdef CONFIG_ATA_SYSFS_COMPAT - char compat_name[64]; -#endif device_initialize(dev); dev->type = &ata_port_type; @@ -323,18 +349,14 @@ int ata_tport_add(struct device *parent, goto tport_link_err; } -#ifdef CONFIG_ATA_SYSFS_COMPAT - sprintf(compat_name, "ata%d", ap->print_id); - error = sysfs_create_link(&parent->kobj, &dev->kobj, compat_name); + error = ata_tport_compat_link_add(ap); if (error) goto compat_link_err; -#endif + return 0; -#ifdef CONFIG_ATA_SYSFS_COMPAT compat_link_err: ata_tlink_delete(&ap->link); -#endif tport_link_err: transport_remove_device(dev); device_del(dev); > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/ata/Kconfig | 10 ++++++++++ > drivers/ata/libata-transport.c | 20 ++++++++++++++++++++ > 2 files changed, 30 insertions(+) > > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig > index e5641e6c52ee..f27b12ba2ce7 100644 > --- a/drivers/ata/Kconfig > +++ b/drivers/ata/Kconfig > @@ -51,6 +51,16 @@ config ATA_VERBOSE_ERROR > > If unsure, say Y. > > +config ATA_SYSFS_COMPAT > + bool "Keep original sysfs layout" > + default y > + help > + This option retains the original sysfs layout by adding an > + additional ata_port object with the name of 'ataX' in > + to the ATA objects like 'ata_port', 'ata_link', and 'ata_device'. > + > + If unsure, say Y. > + > config ATA_FORCE > bool "\"libata.force=\" kernel parameter support" if EXPERT > default y > diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c > index 555fe6e2293d..a66c3480bdcf 100644 > --- a/drivers/ata/libata-transport.c > +++ b/drivers/ata/libata-transport.c > @@ -260,7 +260,13 @@ static int ata_tport_match(struct attribute_container *cont, > void ata_tport_delete(struct ata_port *ap) > { > struct device *dev = &ap->tdev; > +#ifdef CONFIG_ATA_SYSFS_COMPAT > + struct device *parent = dev->parent; > + char compat_name[64]; > > + sprintf(compat_name, "ata%d", ap->print_id); > + sysfs_remove_link(&parent->kobj, compat_name); > +#endif > ata_tlink_delete(&ap->link); > > transport_remove_device(dev); > @@ -284,6 +290,9 @@ int ata_tport_add(struct device *parent, > { > int error; > struct device *dev = &ap->tdev; > +#ifdef CONFIG_ATA_SYSFS_COMPAT > + char compat_name[64]; > +#endif > > device_initialize(dev); > dev->type = &ata_port_type; > @@ -313,8 +322,19 @@ int ata_tport_add(struct device *parent, > if (error) { > goto tport_link_err; > } > + > +#ifdef CONFIG_ATA_SYSFS_COMPAT > + sprintf(compat_name, "ata%d", ap->print_id); > + error = sysfs_create_link(&parent->kobj, &dev->kobj, compat_name); > + if (error) > + goto compat_link_err; > +#endif > return 0; > > +#ifdef CONFIG_ATA_SYSFS_COMPAT > + compat_link_err: > + ata_tlink_delete(&ap->link); > +#endif > tport_link_err: > transport_remove_device(dev); > device_del(dev);
On 3/25/22 04:01, Damien Le Moal wrote: > On 3/24/22 21:32, Hannes Reinecke wrote: >> Add a config option 'ATA_SYSFS_COMPAT' to create a compability > > s/compability/compatibility > >> 'ata' symlink in the PCI device sysfs directory. > > I am not yet convinced if this new config option is really necessary... > We could create the symlink unconditionally, no ? > We could, but why? The sole point of the compat symlink is to preserve compability with previous releases. But we don't really know if this compatility really is required; I haven't seen any difference in behaviour with or without the symlinks. By having a config option we make it clear that the symlinks will eventually vanish. > In any case, I would like to at least reduce the number of #ifdef. So > what about something like this on top of your patch: > Sure. Will be doing so in the next round. Cheers, Hannes
On 2022/03/25 16:12, Hannes Reinecke wrote: > On 3/25/22 04:01, Damien Le Moal wrote: >> On 3/24/22 21:32, Hannes Reinecke wrote: >>> Add a config option 'ATA_SYSFS_COMPAT' to create a compability >> >> s/compability/compatibility >> >>> 'ata' symlink in the PCI device sysfs directory. >> >> I am not yet convinced if this new config option is really necessary... >> We could create the symlink unconditionally, no ? >> > We could, but why? > The sole point of the compat symlink is to preserve compability with > previous releases. But we don't really know if this compatility really > is required; I haven't seen any difference in behaviour with or without > the symlinks. > By having a config option we make it clear that the symlinks will > eventually vanish. OK. The default is "y" for now anyway, so it is safe. > >> In any case, I would like to at least reduce the number of #ifdef. So >> what about something like this on top of your patch: >> > Sure. Will be doing so in the next round. > > Cheers, > > Hannes
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig index e5641e6c52ee..f27b12ba2ce7 100644 --- a/drivers/ata/Kconfig +++ b/drivers/ata/Kconfig @@ -51,6 +51,16 @@ config ATA_VERBOSE_ERROR If unsure, say Y. +config ATA_SYSFS_COMPAT + bool "Keep original sysfs layout" + default y + help + This option retains the original sysfs layout by adding an + additional ata_port object with the name of 'ataX' in + to the ATA objects like 'ata_port', 'ata_link', and 'ata_device'. + + If unsure, say Y. + config ATA_FORCE bool "\"libata.force=\" kernel parameter support" if EXPERT default y diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c index 555fe6e2293d..a66c3480bdcf 100644 --- a/drivers/ata/libata-transport.c +++ b/drivers/ata/libata-transport.c @@ -260,7 +260,13 @@ static int ata_tport_match(struct attribute_container *cont, void ata_tport_delete(struct ata_port *ap) { struct device *dev = &ap->tdev; +#ifdef CONFIG_ATA_SYSFS_COMPAT + struct device *parent = dev->parent; + char compat_name[64]; + sprintf(compat_name, "ata%d", ap->print_id); + sysfs_remove_link(&parent->kobj, compat_name); +#endif ata_tlink_delete(&ap->link); transport_remove_device(dev); @@ -284,6 +290,9 @@ int ata_tport_add(struct device *parent, { int error; struct device *dev = &ap->tdev; +#ifdef CONFIG_ATA_SYSFS_COMPAT + char compat_name[64]; +#endif device_initialize(dev); dev->type = &ata_port_type; @@ -313,8 +322,19 @@ int ata_tport_add(struct device *parent, if (error) { goto tport_link_err; } + +#ifdef CONFIG_ATA_SYSFS_COMPAT + sprintf(compat_name, "ata%d", ap->print_id); + error = sysfs_create_link(&parent->kobj, &dev->kobj, compat_name); + if (error) + goto compat_link_err; +#endif return 0; +#ifdef CONFIG_ATA_SYSFS_COMPAT + compat_link_err: + ata_tlink_delete(&ap->link); +#endif tport_link_err: transport_remove_device(dev); device_del(dev);
Add a config option 'ATA_SYSFS_COMPAT' to create a compability 'ata' symlink in the PCI device sysfs directory. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/ata/Kconfig | 10 ++++++++++ drivers/ata/libata-transport.c | 20 ++++++++++++++++++++ 2 files changed, 30 insertions(+)