| Message ID | 20240902000043.155495-2-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Code cleanup and memory usage reduction | expand |
On 9/2/24 02:00, Damien Le Moal wrote: > Move the transport link device related functions after the device > transport related functions to avoid the need for forward declaring > ata_tdev_add() and ata_tdev_delete(). > > And while at it, improve the kdoc comments for ata_tdev_free() and > ata_tdev_delete(). > > No functional changes are introduced. > > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > --- > drivers/ata/libata-transport.c | 281 ++++++++++++++++----------------- > 1 file changed, 137 insertions(+), 144 deletions(-) > > diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c > index 48800cd0e75d..2a5025b2f28b 100644 > --- a/drivers/ata/libata-transport.c > +++ b/drivers/ata/libata-transport.c > @@ -80,12 +80,6 @@ struct ata_internal { > #define transport_class_to_port(dev) \ > tdev_to_port((dev)->parent) > > - > -/* Device objects are always created whit link objects */ > -static int ata_tdev_add(struct ata_device *dev); > -static void ata_tdev_delete(struct ata_device *dev); > - > - > /* > * Hack to allow attributes of the same name in different objects. > */ > @@ -364,135 +358,6 @@ unsigned int ata_port_classify(struct ata_port *ap, > } > EXPORT_SYMBOL_GPL(ata_port_classify); > > -/* > - * ATA link attributes > - */ > -static int noop(int x) { return x; } > - > -#define ata_link_show_linkspeed(field, format) \ > -static ssize_t \ > -show_ata_link_##field(struct device *dev, \ > - struct device_attribute *attr, char *buf) \ > -{ \ > - struct ata_link *link = transport_class_to_link(dev); \ > - \ > - return sprintf(buf, "%s\n", sata_spd_string(format(link->field))); \ > -} > - > -#define ata_link_linkspeed_attr(field, format) \ > - ata_link_show_linkspeed(field, format) \ > -static DEVICE_ATTR(field, S_IRUGO, show_ata_link_##field, NULL) > - > -ata_link_linkspeed_attr(hw_sata_spd_limit, fls); > -ata_link_linkspeed_attr(sata_spd_limit, fls); > -ata_link_linkspeed_attr(sata_spd, noop); > - > - > -static DECLARE_TRANSPORT_CLASS(ata_link_class, > - "ata_link", NULL, NULL, NULL); > - > -static void ata_tlink_release(struct device *dev) > -{ > -} > - > -/** > - * ata_is_link -- check if a struct device represents a ATA link > - * @dev: device to check > - * > - * Returns: > - * %1 if the device represents a ATA link, %0 else > - */ > -static int ata_is_link(const struct device *dev) > -{ > - return dev->release == ata_tlink_release; > -} > - > -static int ata_tlink_match(struct attribute_container *cont, > - struct device *dev) > -{ > - struct ata_internal* i = to_ata_internal(ata_scsi_transport_template); > - if (!ata_is_link(dev)) > - return 0; > - return &i->link_attr_cont.ac == cont; > -} > - > -/** > - * ata_tlink_delete -- remove ATA LINK > - * @link: ATA LINK to remove > - * > - * Removes the specified ATA LINK. remove associated ATA device(s) as well. > - */ > -void ata_tlink_delete(struct ata_link *link) > -{ > - struct device *dev = &link->tdev; > - struct ata_device *ata_dev; > - > - ata_for_each_dev(ata_dev, link, ALL) { > - ata_tdev_delete(ata_dev); > - } > - > - transport_remove_device(dev); > - device_del(dev); > - transport_destroy_device(dev); > - put_device(dev); > -} > - > -/** > - * ata_tlink_add -- initialize a transport ATA link structure > - * @link: allocated ata_link structure. > - * > - * Initialize an ATA LINK structure for sysfs. It will be added in the > - * device tree below the ATA PORT it belongs to. > - * > - * Returns %0 on success > - */ > -int ata_tlink_add(struct ata_link *link) > -{ > - struct device *dev = &link->tdev; > - struct ata_port *ap = link->ap; > - struct ata_device *ata_dev; > - int error; > - > - device_initialize(dev); > - dev->parent = &ap->tdev; > - dev->release = ata_tlink_release; > - if (ata_is_host_link(link)) > - dev_set_name(dev, "link%d", ap->print_id); > - else > - dev_set_name(dev, "link%d.%d", ap->print_id, link->pmp); > - > - transport_setup_device(dev); > - > - error = device_add(dev); > - if (error) { > - goto tlink_err; > - } > - > - error = transport_add_device(dev); > - if (error) > - goto tlink_transport_err; > - transport_configure_device(dev); > - > - ata_for_each_dev(ata_dev, link, ALL) { > - error = ata_tdev_add(ata_dev); > - if (error) { > - goto tlink_dev_err; > - } > - } > - return 0; > - tlink_dev_err: > - while (--ata_dev >= link->device) { > - ata_tdev_delete(ata_dev); > - } > - transport_remove_device(dev); > - tlink_transport_err: > - device_del(dev); > - tlink_err: > - transport_destroy_device(dev); > - put_device(dev); > - return error; > -} > - > /* > * ATA device attributes > */ > @@ -660,14 +525,14 @@ static int ata_tdev_match(struct attribute_container *cont, > } > > /** > - * ata_tdev_free -- free a ATA LINK > - * @dev: ATA PHY to free > + * ata_tdev_free -- free a transport ATA device structure Odd wording; maybe 'ATA transport device' ? > + * @dev: target ATA device Why 'target ATA device'? Wouldn't 'ATA transport device' be better? > * > - * Frees the specified ATA PHY. > + * Free the transport ATA device structure for the specified ATA device. Same here. > * > * Note: > - * This function must only be called on a PHY that has not > - * successfully been added using ata_tdev_add(). > + * This function must only be called on a device that has not successfully 'device'? Shouldn't we not use the same wording as in the description? > + * been added using ata_tdev_add(). > */ > static void ata_tdev_free(struct ata_device *dev) > { > @@ -676,10 +541,10 @@ static void ata_tdev_free(struct ata_device *dev) > } > > /** > - * ata_tdev_delete -- remove ATA device > - * @ata_dev: ATA device to remove > + * ata_tdev_delete -- remove an ATA device sysfs entry > + * @ata_dev: target ATA device > * And here we should be consistent with whatever wording we've been using in ata_tdev_free(). > - * Removes the specified ATA device. > + * Removes the transport sysfs entry for the specified ATA device. > */ > static void ata_tdev_delete(struct ata_device *ata_dev) > { > @@ -690,7 +555,6 @@ static void ata_tdev_delete(struct ata_device *ata_dev) > ata_tdev_free(ata_dev); > } > > - > /** > * ata_tdev_add -- initialize a transport ATA device structure. > * @ata_dev: ata_dev structure. And we would need to modify this, too, to have the same wording. > @@ -734,6 +598,135 @@ static int ata_tdev_add(struct ata_device *ata_dev) > return 0; > } > > +/* > + * ATA link attributes > + */ > +static int noop(int x) > +{ > + return x; > +} > + > +#define ata_link_show_linkspeed(field, format) \ > +static ssize_t \ > +show_ata_link_##field(struct device *dev, \ > + struct device_attribute *attr, char *buf) \ > +{ \ > + struct ata_link *link = transport_class_to_link(dev); \ > + \ > + return sprintf(buf, "%s\n", \ > + sata_spd_string(format(link->field))); \ > +} > + > +#define ata_link_linkspeed_attr(field, format) \ > + ata_link_show_linkspeed(field, format) \ > +static DEVICE_ATTR(field, 0444, show_ata_link_##field, NULL) > + > +ata_link_linkspeed_attr(hw_sata_spd_limit, fls); > +ata_link_linkspeed_attr(sata_spd_limit, fls); > +ata_link_linkspeed_attr(sata_spd, noop); > + > +static DECLARE_TRANSPORT_CLASS(ata_link_class, > + "ata_link", NULL, NULL, NULL); > + > +static void ata_tlink_release(struct device *dev) > +{ > +} > + > +/** > + * ata_is_link -- check if a struct device represents a ATA link > + * @dev: device to check > + * > + * Returns: > + * %1 if the device represents a ATA link, %0 else > + */ > +static int ata_is_link(const struct device *dev) Why is this not a boolean ? > +{ > + return dev->release == ata_tlink_release; > +} > + > +static int ata_tlink_match(struct attribute_container *cont, > + struct device *dev) > +{ > + struct ata_internal *i = to_ata_internal(ata_scsi_transport_template); > + > + if (!ata_is_link(dev)) > + return 0; > + return &i->link_attr_cont.ac == cont; > +} > + > +/** > + * ata_tlink_delete -- remove ATA LINK > + * @link: ATA LINK to remove Why is the 'link' in capital letters? > + * > + * Removes the specified ATA LINK. remove associated ATA device(s) as well. > + */ > +void ata_tlink_delete(struct ata_link *link) > +{ > + struct device *dev = &link->tdev; > + struct ata_device *ata_dev; > + > + ata_for_each_dev(ata_dev, link, ALL) { > + ata_tdev_delete(ata_dev); > + } > + > + transport_remove_device(dev); > + device_del(dev); > + transport_destroy_device(dev); > + put_device(dev); > +} > + > +/** > + * ata_tlink_add -- initialize a transport ATA link structure > + * @link: allocated ata_link structure. Same comment than above: why 'transport ATA link', and not 'ATA transport link' ? > + * > + * Initialize an ATA LINK structure for sysfs. It will be added in the > + * device tree below the ATA PORT it belongs to. > + * > + * Returns %0 on success And what on failure? > + */ > +int ata_tlink_add(struct ata_link *link) > +{ > + struct device *dev = &link->tdev; > + struct ata_port *ap = link->ap; > + struct ata_device *ata_dev; > + int error; > + > + device_initialize(dev); > + dev->parent = &ap->tdev; > + dev->release = ata_tlink_release; > + if (ata_is_host_link(link)) > + dev_set_name(dev, "link%d", ap->print_id); > + else > + dev_set_name(dev, "link%d.%d", ap->print_id, link->pmp); > + > + transport_setup_device(dev); > + > + error = device_add(dev); > + if (error) > + goto tlink_err; > + > + error = transport_add_device(dev); > + if (error) > + goto tlink_transport_err; > + transport_configure_device(dev); > + > + ata_for_each_dev(ata_dev, link, ALL) { > + error = ata_tdev_add(ata_dev); > + if (error) > + goto tlink_dev_err; > + } > + return 0; > + tlink_dev_err: > + while (--ata_dev >= link->device) > + ata_tdev_delete(ata_dev); > + transport_remove_device(dev); > + tlink_transport_err: > + device_del(dev); > + tlink_err: > + transport_destroy_device(dev); > + put_device(dev); > + return error; > +} > > /* > * Setup / Teardown code Cheers, Hannes
On 9/2/24 15:16, Hannes Reinecke wrote: >> /* >> * ATA device attributes >> */ >> @@ -660,14 +525,14 @@ static int ata_tdev_match(struct attribute_container *cont, >> } >> >> /** >> - * ata_tdev_free -- free a ATA LINK >> - * @dev: ATA PHY to free >> + * ata_tdev_free -- free a transport ATA device structure > > Odd wording; maybe 'ATA transport device' ? Indeed. > >> + * @dev: target ATA device > > Why 'target ATA device'? Wouldn't 'ATA transport device' be better? Because that function really takes a pointer to struct ata_dev, not to struct ata_dev->tdev... > >> * >> - * Frees the specified ATA PHY. >> + * Free the transport ATA device structure for the specified ATA device. > > Same here. > >> * >> * Note: >> - * This function must only be called on a PHY that has not >> - * successfully been added using ata_tdev_add(). >> + * This function must only be called on a device that has not successfully > > 'device'? Shouldn't we not use the same wording as in the description? Not really. Here the reference is to the struct ata_device. Will clarify that. > >> + * been added using ata_tdev_add(). >> */ >> static void ata_tdev_free(struct ata_device *dev) >> { >> @@ -676,10 +541,10 @@ static void ata_tdev_free(struct ata_device *dev) >> } >> >> /** >> - * ata_tdev_delete -- remove ATA device >> - * @ata_dev: ATA device to remove >> + * ata_tdev_delete -- remove an ATA device sysfs entry >> + * @ata_dev: target ATA device >> * > > And here we should be consistent with whatever wording we've been using > in ata_tdev_free(). Yep, will fix. >> +/** >> + * ata_is_link -- check if a struct device represents a ATA link >> + * @dev: device to check >> + * >> + * Returns: >> + * %1 if the device represents a ATA link, %0 else >> + */ >> +static int ata_is_link(const struct device *dev) > > Why is this not a boolean ? It was like that. I can make it a boolean. > >> +{ >> + return dev->release == ata_tlink_release; >> +} >> + >> +static int ata_tlink_match(struct attribute_container *cont, >> + struct device *dev) >> +{ >> + struct ata_internal *i = to_ata_internal(ata_scsi_transport_template); >> + >> + if (!ata_is_link(dev)) >> + return 0; >> + return &i->link_attr_cont.ac == cont; >> +} >> + >> +/** >> + * ata_tlink_delete -- remove ATA LINK >> + * @link: ATA LINK to remove > > Why is the 'link' in capital letters? No idea. It was like that. Will clean that up as well. >> + * >> + * Initialize an ATA LINK structure for sysfs. It will be added in the >> + * device tree below the ATA PORT it belongs to. >> + * >> + * Returns %0 on success > > And what on failure? Another one that I did not touch but clearly needs cleanup too :)
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c index 48800cd0e75d..2a5025b2f28b 100644 --- a/drivers/ata/libata-transport.c +++ b/drivers/ata/libata-transport.c @@ -80,12 +80,6 @@ struct ata_internal { #define transport_class_to_port(dev) \ tdev_to_port((dev)->parent) - -/* Device objects are always created whit link objects */ -static int ata_tdev_add(struct ata_device *dev); -static void ata_tdev_delete(struct ata_device *dev); - - /* * Hack to allow attributes of the same name in different objects. */ @@ -364,135 +358,6 @@ unsigned int ata_port_classify(struct ata_port *ap, } EXPORT_SYMBOL_GPL(ata_port_classify); -/* - * ATA link attributes - */ -static int noop(int x) { return x; } - -#define ata_link_show_linkspeed(field, format) \ -static ssize_t \ -show_ata_link_##field(struct device *dev, \ - struct device_attribute *attr, char *buf) \ -{ \ - struct ata_link *link = transport_class_to_link(dev); \ - \ - return sprintf(buf, "%s\n", sata_spd_string(format(link->field))); \ -} - -#define ata_link_linkspeed_attr(field, format) \ - ata_link_show_linkspeed(field, format) \ -static DEVICE_ATTR(field, S_IRUGO, show_ata_link_##field, NULL) - -ata_link_linkspeed_attr(hw_sata_spd_limit, fls); -ata_link_linkspeed_attr(sata_spd_limit, fls); -ata_link_linkspeed_attr(sata_spd, noop); - - -static DECLARE_TRANSPORT_CLASS(ata_link_class, - "ata_link", NULL, NULL, NULL); - -static void ata_tlink_release(struct device *dev) -{ -} - -/** - * ata_is_link -- check if a struct device represents a ATA link - * @dev: device to check - * - * Returns: - * %1 if the device represents a ATA link, %0 else - */ -static int ata_is_link(const struct device *dev) -{ - return dev->release == ata_tlink_release; -} - -static int ata_tlink_match(struct attribute_container *cont, - struct device *dev) -{ - struct ata_internal* i = to_ata_internal(ata_scsi_transport_template); - if (!ata_is_link(dev)) - return 0; - return &i->link_attr_cont.ac == cont; -} - -/** - * ata_tlink_delete -- remove ATA LINK - * @link: ATA LINK to remove - * - * Removes the specified ATA LINK. remove associated ATA device(s) as well. - */ -void ata_tlink_delete(struct ata_link *link) -{ - struct device *dev = &link->tdev; - struct ata_device *ata_dev; - - ata_for_each_dev(ata_dev, link, ALL) { - ata_tdev_delete(ata_dev); - } - - transport_remove_device(dev); - device_del(dev); - transport_destroy_device(dev); - put_device(dev); -} - -/** - * ata_tlink_add -- initialize a transport ATA link structure - * @link: allocated ata_link structure. - * - * Initialize an ATA LINK structure for sysfs. It will be added in the - * device tree below the ATA PORT it belongs to. - * - * Returns %0 on success - */ -int ata_tlink_add(struct ata_link *link) -{ - struct device *dev = &link->tdev; - struct ata_port *ap = link->ap; - struct ata_device *ata_dev; - int error; - - device_initialize(dev); - dev->parent = &ap->tdev; - dev->release = ata_tlink_release; - if (ata_is_host_link(link)) - dev_set_name(dev, "link%d", ap->print_id); - else - dev_set_name(dev, "link%d.%d", ap->print_id, link->pmp); - - transport_setup_device(dev); - - error = device_add(dev); - if (error) { - goto tlink_err; - } - - error = transport_add_device(dev); - if (error) - goto tlink_transport_err; - transport_configure_device(dev); - - ata_for_each_dev(ata_dev, link, ALL) { - error = ata_tdev_add(ata_dev); - if (error) { - goto tlink_dev_err; - } - } - return 0; - tlink_dev_err: - while (--ata_dev >= link->device) { - ata_tdev_delete(ata_dev); - } - transport_remove_device(dev); - tlink_transport_err: - device_del(dev); - tlink_err: - transport_destroy_device(dev); - put_device(dev); - return error; -} - /* * ATA device attributes */ @@ -660,14 +525,14 @@ static int ata_tdev_match(struct attribute_container *cont, } /** - * ata_tdev_free -- free a ATA LINK - * @dev: ATA PHY to free + * ata_tdev_free -- free a transport ATA device structure + * @dev: target ATA device * - * Frees the specified ATA PHY. + * Free the transport ATA device structure for the specified ATA device. * * Note: - * This function must only be called on a PHY that has not - * successfully been added using ata_tdev_add(). + * This function must only be called on a device that has not successfully + * been added using ata_tdev_add(). */ static void ata_tdev_free(struct ata_device *dev) { @@ -676,10 +541,10 @@ static void ata_tdev_free(struct ata_device *dev) } /** - * ata_tdev_delete -- remove ATA device - * @ata_dev: ATA device to remove + * ata_tdev_delete -- remove an ATA device sysfs entry + * @ata_dev: target ATA device * - * Removes the specified ATA device. + * Removes the transport sysfs entry for the specified ATA device. */ static void ata_tdev_delete(struct ata_device *ata_dev) { @@ -690,7 +555,6 @@ static void ata_tdev_delete(struct ata_device *ata_dev) ata_tdev_free(ata_dev); } - /** * ata_tdev_add -- initialize a transport ATA device structure. * @ata_dev: ata_dev structure. @@ -734,6 +598,135 @@ static int ata_tdev_add(struct ata_device *ata_dev) return 0; } +/* + * ATA link attributes + */ +static int noop(int x) +{ + return x; +} + +#define ata_link_show_linkspeed(field, format) \ +static ssize_t \ +show_ata_link_##field(struct device *dev, \ + struct device_attribute *attr, char *buf) \ +{ \ + struct ata_link *link = transport_class_to_link(dev); \ + \ + return sprintf(buf, "%s\n", \ + sata_spd_string(format(link->field))); \ +} + +#define ata_link_linkspeed_attr(field, format) \ + ata_link_show_linkspeed(field, format) \ +static DEVICE_ATTR(field, 0444, show_ata_link_##field, NULL) + +ata_link_linkspeed_attr(hw_sata_spd_limit, fls); +ata_link_linkspeed_attr(sata_spd_limit, fls); +ata_link_linkspeed_attr(sata_spd, noop); + +static DECLARE_TRANSPORT_CLASS(ata_link_class, + "ata_link", NULL, NULL, NULL); + +static void ata_tlink_release(struct device *dev) +{ +} + +/** + * ata_is_link -- check if a struct device represents a ATA link + * @dev: device to check + * + * Returns: + * %1 if the device represents a ATA link, %0 else + */ +static int ata_is_link(const struct device *dev) +{ + return dev->release == ata_tlink_release; +} + +static int ata_tlink_match(struct attribute_container *cont, + struct device *dev) +{ + struct ata_internal *i = to_ata_internal(ata_scsi_transport_template); + + if (!ata_is_link(dev)) + return 0; + return &i->link_attr_cont.ac == cont; +} + +/** + * ata_tlink_delete -- remove ATA LINK + * @link: ATA LINK to remove + * + * Removes the specified ATA LINK. remove associated ATA device(s) as well. + */ +void ata_tlink_delete(struct ata_link *link) +{ + struct device *dev = &link->tdev; + struct ata_device *ata_dev; + + ata_for_each_dev(ata_dev, link, ALL) { + ata_tdev_delete(ata_dev); + } + + transport_remove_device(dev); + device_del(dev); + transport_destroy_device(dev); + put_device(dev); +} + +/** + * ata_tlink_add -- initialize a transport ATA link structure + * @link: allocated ata_link structure. + * + * Initialize an ATA LINK structure for sysfs. It will be added in the + * device tree below the ATA PORT it belongs to. + * + * Returns %0 on success + */ +int ata_tlink_add(struct ata_link *link) +{ + struct device *dev = &link->tdev; + struct ata_port *ap = link->ap; + struct ata_device *ata_dev; + int error; + + device_initialize(dev); + dev->parent = &ap->tdev; + dev->release = ata_tlink_release; + if (ata_is_host_link(link)) + dev_set_name(dev, "link%d", ap->print_id); + else + dev_set_name(dev, "link%d.%d", ap->print_id, link->pmp); + + transport_setup_device(dev); + + error = device_add(dev); + if (error) + goto tlink_err; + + error = transport_add_device(dev); + if (error) + goto tlink_transport_err; + transport_configure_device(dev); + + ata_for_each_dev(ata_dev, link, ALL) { + error = ata_tdev_add(ata_dev); + if (error) + goto tlink_dev_err; + } + return 0; + tlink_dev_err: + while (--ata_dev >= link->device) + ata_tdev_delete(ata_dev); + transport_remove_device(dev); + tlink_transport_err: + device_del(dev); + tlink_err: + transport_destroy_device(dev); + put_device(dev); + return error; +} /* * Setup / Teardown code
Move the transport link device related functions after the device transport related functions to avoid the need for forward declaring ata_tdev_add() and ata_tdev_delete(). And while at it, improve the kdoc comments for ata_tdev_free() and ata_tdev_delete(). No functional changes are introduced. Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/ata/libata-transport.c | 281 ++++++++++++++++----------------- 1 file changed, 137 insertions(+), 144 deletions(-)