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@xxxxxxxxxx>
---
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
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@xxxxxxx +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
