Code cleanup and memory usage reduction
Damien Le Moal
maintainerv3 cover Code cleanup and memory usage reduction
Patch 1 of this series cleans up libata-transport (avoid forward declarations and improved kdoc comments).
Patch 2 introduces a small simplification/improvement of __ata_qc_complete().
Patches 3 and 4 move code that is SATA specific from libata-core.c to libata-sata.c, without any functional change. The benefits of this code reorganization is a smaller libata binary size for hosts that do not support SATA.
Patch 5 renames some functions to make it clearer what the functions do.
Finally, patch 6 and 7 reduce memory usage of libata by:
- Moving the sector_buf buffer from struct ata_port to struct ata_device
- Agregating CDL related buffers together into a new ata_cdl structure and referencing this new structure from struct ata_device only for devices that support CDL.
Changes from V2:
- Reworked patch 1 to address Hannes' comments
- Added Hannes' review tags
Changes from V1:
- Reworked patch 1 to do more cleanups
- Added patch 6. The former patch 6 of v1 is now squashed into patch 7
- Added Niklas review tags
Damien Le Moal (7):
ata: libata: Cleanup libata-transport
ata: libata: Improve __ata_qc_complete()
ata: libata: Move sata_down_spd_limit() to libata-sata.c
ata: libata: Move sata_std_hardreset() definition to libata-sata.c
ata: libata: Rename ata_eh_read_sense_success_ncq_log()
ata: libata: Move sector_buf from struct ata_port to struct ata_device
ata: libata: Improve CDL resource management
drivers/ata/libata-core.c | 265 +++++++++-------------------- drivers/ata/libata-eh.c | 10 +- drivers/ata/libata-pmp.c | 3 +- drivers/ata/libata-sata.c | 127 +++++++++++++- drivers/ata/libata-scsi.c | 2 +- drivers/ata/libata-transport.c | 299 ++++++++++++++++----------------- drivers/ata/libata-zpodd.c | 2 +- drivers/ata/libata.h | 23 ++- include/linux/libata.h | 39 +++-- 9 files changed, 407 insertions(+), 363 deletions(-)
Signature · 1 line
2.46.0
Damien Le Moal
maintainerPATCH v3 1/7 ata: libata: Cleanup libata-transport
Move the ATA link transport device related functions after the ATA transport device related functions to avoid the need for forward declaring ata_tdev_add() and ata_tdev_delete().
And while at it, do the following:
1) Change ata_is_ata_dev() and ata_is_link() to return a boolean
2) Fix a pointer declaration style in ata_is_ata_dev()
3) Improve the kdoc comments for ata_tdev_free(), ata_tdev_delete(), ata_tdev_add(), ata_tlink_delete() and ata_tlink_add()
No functional changes are introduced by this cleanup.
--- drivers/ata/libata-transport.c | 299 ++++++++++++++++----------------- 1 file changed, 147 insertions(+), 152 deletions(-)
drivers/ata/libata-transport.c +147 -152
@@ -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 */
@@ -643,9 +508,9 @@ static void ata_tdev_release(struct device *dev)* @dev: device to check * * Returns: -* %1 if the device represents a ATA device, %0 else +* true if the device represents a ATA device, false otherwise */ -static int ata_is_ata_dev (const struct device *dev) +static bool ata_is_ata_dev (const struct device *dev) {return dev->release == ata_tdev_release; }
@@ -653,21 +518,22 @@ static int ata_is_ata_dev(const struct device *dev)static int ata_tdev_match (struct attribute_container *cont,struct device *dev) { -struct ata_internal * i =to_ata_internal (ata_scsi_transport_template); +struct ata_internal *i =to_ata_internal (ata_scsi_transport_template); +if (!ata_is_ata_dev (dev))return 0 ;return &i->dev_attr_cont.ac == cont; }/** -* ata_tdev_free -- free a ATA LINK -* @dev: ATA PHY to free +* ata_tdev_free -- free an ATA transport device +* @dev: struct ata_device owning the transport device to free * -* Frees the specified ATA PHY. +* Free the ATA transport device 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 for a ATA transport device that has not +* yet successfully been added using ata_tdev_add(). */ static void ata_tdev_free (struct ata_device *dev) {
@@ -676,10 +542,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 transport device +* @ata_dev: struct ata_device owning the transport device to delete * -* Removes the specified ATA device. +* Removes the ATA transport device for the specified ATA device. */ static void ata_tdev_delete (struct ata_device *ata_dev) {
@@ -690,15 +556,14 @@ 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. +* ata_tdev_add -- initialize an ATA transport device +* @ata_dev: struct ata_device owning the transport device to add * -* Initialize an ATA device structure for sysfs. It will be added in the -* device tree below the ATA LINK device it belongs to. +* Initialize an ATA transport device for sysfs. It will be added in the +* device tree below the ATA link device it belongs to. * -* Returns %0 on success +* Returns %0 on success and a negative error code on error. */ static int ata_tdev_add (struct ata_device *ata_dev) {
@@ -734,6 +599,136 @@ 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: +* true if the device represents a ATA link, false otherwise +*/ +static bool 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 an ATA link transport device +* @link: struct ata_link owning the link transport device to remove +* +* Removes the link transport device of the specified ATA link. This also +* removes the ATA device(s) associated with the link 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 an ATA link transport device +* @ata_link: struct ata_link owning the link transport device to initialize +* +* Initialize an ATA link transport device for sysfs. It will be added in the +* device tree below the ATA port it belongs to. +* +* Returns %0 on success and a negative error code on error. +*/ +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
Signature · 1 line
2.46.0
Re: [PATCH v3 1/7] ata: libata: Cleanup libata-transport
Quoted 10 lines · Damien Le Moal
Move the ATA link transport device related functions after the ATA transport device related functions to avoid the need for forward declaring
ata_tdev_add()andata_tdev_delete().And while at it, do the following:
1) Changeata_is_ata_dev()andata_is_link()to return a boolean
2) Fix a pointer declaration style inata_is_ata_dev()
3) Improve the kdoc comments forata_tdev_free(),ata_tdev_delete(),ata_tdev_add(),ata_tlink_delete()andata_tlink_add()
Quoted 7 lines
No functional changes are introduced by this cleanup.
Signed-off-by Damien Le Moal <[email protected]>--- drivers/ata/libata-transport.c | 299 ++++++++++++++++----------------- 1 file changed, 147 insertions(+), 152 deletions(-)
Cheers,
Hannes
Signature · 4 lines
Dr. Hannes Reinecke Kernel Storage Architect [email protected] +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
Re: [PATCH v3 1/7] ata: libata: Cleanup libata-transport
In reply to Damien Le MoalQuoted 10 lines · Damien Le Moal
Move the ATA link transport device related functions after the ATA transport device related functions to avoid the need for forward declaring
ata_tdev_add()andata_tdev_delete().And while at it, do the following:
1) Changeata_is_ata_dev()andata_is_link()to return a boolean
2) Fix a pointer declaration style inata_is_ata_dev()
3) Improve the kdoc comments forata_tdev_free(),ata_tdev_delete(),ata_tdev_add(),ata_tlink_delete()andata_tlink_add()
Quoted 4 lines
No functional changes are introduced by this cleanup.
Signed-off-by Damien Le Moal <[email protected]>---
Damien Le Moal
maintainerPATCH v3 2/7 ata: libata: Improve __ata_qc_complete()
In reply to Damien Le MoalThe function __ata_qc_complete() is always called with a qc that already has been dereferenced and so is guaranteed to be non-NULL (as otherwise the kernel would have crashed). So remove the warning for a NULL qc as it is useless.
Furthermore, the qc passed to __ata_qc_complete() must always be marked as active with the ATA_QCFLAG_ACTIVE flag. If that is not the case, in addition to the existing warning, return early so that we do not attempt to complete an invalid qc.
Finally, fix the comment related to clearing the qc active flag as that operation applies to all devices, not just ATAPI ones.
--- drivers/ata/libata-core.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/ata/libata-core.c +7 -5
@@ -4829,8 +4829,9 @@ void __ata_qc_complete(struct ata_queued_cmd *qc)struct ata_port *ap;struct ata_link *link; -WARN_ON_ONCE (qc ==NULL );/* ata_qc_from_tag _might_ return NULL */ -WARN_ON_ONCE (!(qc->flags &ATA_QCFLAG_ACTIVE )); +if (WARN_ON_ONCE (!(qc->flags &ATA_QCFLAG_ACTIVE ))) +return ; + ap = qc->ap; link = qc->dev->link;
@@ -4852,9 +4853,10 @@ void __ata_qc_complete(struct ata_queued_cmd *qc) ap->excl_link == link)) ap->excl_link =NULL ; -/* atapi: mark qc as inactive to prevent the interrupt handler -* from completing the command twice later, before the error handler -* is called. (when rc != 0 and atapi request sense is needed) +/* +* Mark qc as inactive to prevent the port interrupt handler from +* completing the command twice later, before the error handler is +* called. */ qc->flags &= ~ATA_QCFLAG_ACTIVE ; ap->qc_active &= ~(1ULL << qc->tag);
Signature · 1 line
2.46.0
Damien Le Moal
maintainerPATCH v3 3/7 ata: libata: Move sata_down_spd_limit() to libata-sata.c
In reply to Damien Le MoalMove the definition of the function sata_down_spd_limit() to libata-sata.c where it belongs, together with sata_set_spd(). The helper function ata_sstatus_online() is also changed to be an inline function defined in drivers/ata/libata.h.
--- drivers/ata/libata-core.c | 85 --------------------------------------- drivers/ata/libata-sata.c | 80 ++++++++++++++++++++++++++++++++++++ drivers/ata/libata.h | 17 +++++++- 3 files changed, 96 insertions(+), 86 deletions(-)
drivers/ata/libata-core.c +0 -85
@@ -167,11 +167,6 @@ static inline bool ata_dev_print_info(const struct ata_device *dev)return ehc->i.flags &ATA_EHI_PRINTINFO ; } -static bool ata_sstatus_online (u32 sstatus) -{ -return (sstatus &0xf ) ==0x3 ; -} -/** * ata_link_next - link iteration helper * @link: the previous link, NULL to start
@@ -3200,86 +3195,6 @@ struct ata_device *ata_dev_pair(struct ata_device *adev) }EXPORT_SYMBOL_GPL (ata_dev_pair); -/** -* sata_down_spd_limit - adjust SATA spd limit downward -* @link: Link to adjust SATA spd limit for -* @spd_limit: Additional limit -* -* Adjust SATA spd limit of @link downward. Note that this -* function only adjusts the limit. The change must be applied -* using sata_set_spd(). -* -* If @spd_limit is non-zero, the speed is limited to equal to or -* lower than @spd_limit if such speed is supported. If -* @spd_limit is slower than any supported speed, only the lowest -* supported speed is allowed. -* -* LOCKING: -* Inherited from caller. -* -* RETURNS: -* 0 on success, negative errno on failure -*/ -int sata_down_spd_limit (struct ata_link *link,u32 spd_limit) -{ -u32 sstatus, spd, mask; -int rc, bit; - -if (!sata_scr_valid (link)) -return -EOPNOTSUPP ; - -/* If SCR can be read, use it to determine the current SPD. -* If not, use cached value in link->sata_spd. -*/ - rc =sata_scr_read (link,SCR_STATUS , &sstatus); -if (rc ==0 &&ata_sstatus_online (sstatus)) - spd = (sstatus >>4 ) &0xf ; -else - spd = link->sata_spd; - - mask = link->sata_spd_limit; -if (mask <=1 ) -return -EINVAL ; - -/* unconditionally mask off the highest bit */ - bit =fls (mask) -1 ; - mask &= ~(1 << bit); - -/* -* Mask off all speeds higher than or equal to the current one. At -* this point, if current SPD is not available and we previously -* recorded the link speed from SStatus, the driver has already -* masked off the highest bit so mask should already be 1 or 0. -* Otherwise, we should not force 1.5Gbps on a link where we have -* not previously recorded speed from SStatus. Just return in this -* case. -*/ -if (spd >1 ) - mask &= (1 << (spd -1 )) -1 ; -else if (link->sata_spd) -return -EINVAL ; - -/* were we already at the bottom? */ -if (!mask) -return -EINVAL ; - -if (spd_limit) { -if (mask & ((1 << spd_limit) -1 )) - mask &= (1 << spd_limit) -1 ; -else { - bit =ffs (mask) -1 ; - mask =1 << bit; - } - } - - link->sata_spd_limit = mask; - -ata_link_warn (link,"limiting SATA link speed to %s\n" , -sata_spd_string (fls (mask))); - -return 0 ; -} -#ifdef CONFIG_ATA_ACPI /** * ata_timing_cycle2mode - find xfer mode for the specified cycle duration
drivers/ata/libata-sata.c +80 -0
86 lines not on this page · raw message
drivers/ata/libata.h +16 -1
@@ -38,6 +38,12 @@ extern int libata_noacpi;extern int libata_allow_tpm;extern const struct device_type ata_port_type;extern struct ata_link *ata_dev_phys_link (struct ata_device *dev); + +static inline bool ata_sstatus_online (u32 sstatus) +{ +return (sstatus &0xf ) ==0x3 ; +} +#ifdef CONFIG_ATA_FORCE extern void ata_force_cbl (struct ata_port *ap);#else
@@ -65,7 +71,6 @@ extern bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf, bool set_active);extern void ata_dev_power_set_standby (struct ata_device *dev);extern void ata_dev_power_set_active (struct ata_device *dev); -extern int sata_down_spd_limit (struct ata_link *link,u32 spd_limit);extern int ata_down_xfermask_limit (struct ata_device *dev,unsigned int sel);extern unsigned int ata_dev_set_feature (struct ata_device *dev,u8 subcmd,u8 action);
@@ -87,6 +92,16 @@ extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,#define to_ata_port (d) container_of(d, struct ata_port, tdev) +/* libata-sata.c */ +#ifdef CONFIG_SATA_HOST +int sata_down_spd_limit (struct ata_link *link,u32 spd_limit); +#else +static inline int sata_down_spd_limit (struct ata_link *link,u32 spd_limit) +{ +return -EOPNOTSUPP ; +} +#endif +/* libata-acpi.c */ #ifdef CONFIG_ATA_ACPI extern unsigned int ata_acpi_gtf_filter;
Signature · 1 line
2.46.0
Damien Le Moal
maintainerPATCH v3 4/7 ata: libata: Move sata_std_hardreset() definition to libata-sata.c
In reply to Damien Le MoalUnlike ata_std_prereset() and ata_std_postreset(), the function sata_std_hardreset() applies only to SATA devices, as its name implies. So move its definition to libata-sata.c.
Together with this, also move the definition of sata_port_ops to libata-sata.c, where it belongs.
--- drivers/ata/libata-core.c | 35 ----------------------------------- drivers/ata/libata-sata.c | 36 ++++++++++++++++++++++++++++++++++++ include/linux/libata.h | 9 +++++++-- 3 files changed, 43 insertions(+), 37 deletions(-)
drivers/ata/libata-core.c +0 -35
@@ -72,14 +72,6 @@ const struct ata_port_operations ata_base_port_ops = { .end_eh = ata_std_end_eh, }; -const struct ata_port_operations sata_port_ops = { - .inherits = &ata_base_port_ops, - - .qc_defer = ata_std_qc_defer, - .hardreset = sata_std_hardreset, -}; -EXPORT_SYMBOL_GPL (sata_port_ops); -static unsigned int ata_dev_init_params (struct ata_device *dev,u16 heads,u16 sectors);static unsigned int ata_dev_set_xfermode (struct ata_device *dev);
@@ -3676,33 +3668,6 @@ int ata_std_prereset(struct ata_link *link, unsigned long deadline) }EXPORT_SYMBOL_GPL (ata_std_prereset); -/** -* sata_std_hardreset - COMRESET w/o waiting or classification -* @link: link to reset -* @class: resulting class of attached device -* @deadline: deadline jiffies for the operation -* -* Standard SATA COMRESET w/o waiting or classification. -* -* LOCKING: -* Kernel thread context (may sleep) -* -* RETURNS: -* 0 if link offline, -EAGAIN if link online, -errno on errors. -*/ -int sata_std_hardreset (struct ata_link *link,unsigned int *class, -unsigned long deadline) -{ -const unsigned int *timing =sata_ehc_deb_timing (&link->eh_context); -bool online; -int rc; - -/* do hardreset */ - rc =sata_link_hardreset (link, timing, deadline, &online,NULL ); -return online ? -EAGAIN : rc; -} -EXPORT_SYMBOL_GPL (sata_std_hardreset); -/** * ata_std_postreset - standard postreset callback * @link: the target ata_link
drivers/ata/libata-sata.c +36 -0
@@ -706,6 +706,34 @@ int sata_link_hardreset(struct ata_link *link, const unsigned int *timing, }EXPORT_SYMBOL_GPL (sata_link_hardreset); +/** +* sata_std_hardreset - COMRESET w/o waiting or classification +* @link: link to reset +* @class: resulting class of attached device +* @deadline: deadline jiffies for the operation +* +* Standard SATA COMRESET w/o waiting or classification. +* +* LOCKING: +* Kernel thread context (may sleep) +* +* RETURNS: +* 0 if link offline, -EAGAIN if link online, -errno on errors. +*/ +int sata_std_hardreset (struct ata_link *link,unsigned int *class, +unsigned long deadline) +{ +const unsigned int *timing =sata_ehc_deb_timing (&link->eh_context); +bool online; +int rc; + + rc =sata_link_hardreset (link, timing, deadline, &online,NULL ); +if (online) +return -EAGAIN ; +return rc; +} +EXPORT_SYMBOL_GPL (sata_std_hardreset); +/** * ata_qc_complete_multiple - Complete multiple qcs successfully * @ap: port in question
@@ -1654,3 +1682,11 @@ void ata_eh_analyze_ncq_error(struct ata_link *link) ehc->i.err_mask &= ~AC_ERR_DEV ; }EXPORT_SYMBOL_GPL (ata_eh_analyze_ncq_error); + +const struct ata_port_operations sata_port_ops = { + .inherits = &ata_base_port_ops, + + .qc_defer = ata_std_qc_defer, + .hardreset = sata_std_hardreset, +}; +EXPORT_SYMBOL_GPL (sata_port_ops);
include/linux/libata.h +7 -2
@@ -1104,8 +1104,6 @@ static inline bool ata_port_is_frozen(const struct ata_port *ap)extern int ata_std_prereset (struct ata_link *link,unsigned long deadline);extern int ata_wait_after_reset (struct ata_link *link,unsigned long deadline,int (*check_ready)(struct ata_link *link)); -extern int sata_std_hardreset (struct ata_link *link,unsigned int *class, -unsigned long deadline);extern void ata_std_postreset (struct ata_link *link,unsigned int *classes);extern struct ata_host *ata_host_alloc (struct device *dev,int n_ports);
@@ -1229,6 +1227,8 @@ extern int sata_scr_read(struct ata_link *link, int reg, u32 *val);extern int sata_scr_write (struct ata_link *link,int reg,u32 val);extern int sata_scr_write_flush (struct ata_link *link,int reg,u32 val);extern int sata_set_spd (struct ata_link *link); +int sata_std_hardreset (struct ata_link *link,unsigned int *class, +unsigned long deadline);extern int sata_link_hardreset (struct ata_link *link,const unsigned int *timing,unsigned long deadline,bool *online,int (*check_ready)(struct ata_link *));
@@ -1256,6 +1256,11 @@ static inline int sata_scr_write_flush(struct ata_link *link, int reg, u32 val)return -EOPNOTSUPP ; }static inline int sata_set_spd (struct ata_link *link) {return -EOPNOTSUPP ; } +static inline int sata_std_hardreset (struct ata_link *link,unsigned int *class, +unsigned long deadline) +{ +return -EOPNOTSUPP ; +} static inline int sata_link_hardreset(struct ata_link *link, const unsigned int *timing, unsigned long deadline,
Signature · 1 line
2.46.0
Damien Le Moal
maintainerPATCH v3 5/7 ata: libata: Rename ata_eh_read_sense_success_ncq_log()
In reply to Damien Le MoalThe function ata_eh_read_sense_success_ncq_log() does more that just reading the sense data for successful NCQ commands log page as it also sets the sense data for all commands listed in the log page.
Rename this function to ata_eh_get_ncq_success_sense() to better describe what the function does. Furthermore, since this function is only called from ata_eh_get_success_sense() in libata-eh.c, there is no need to export it and its declaration can be moved to drivers/ata/libata.h.
To be consistent with this change, the function ata_eh_read_sense_success_non_ncq() is also renamed to ata_eh_get_non_ncq_success_sense().
--- drivers/ata/libata-eh.c | 6 +++--- drivers/ata/libata-sata.c | 7 +++---- drivers/ata/libata.h | 5 +++++ include/linux/libata.h | 5 ----- 4 files changed, 11 insertions(+), 12 deletions(-)
drivers/ata/libata-eh.c +3 -3
@@ -1962,7 +1962,7 @@ static inline bool ata_eh_quiet(struct ata_queued_cmd *qc)return qc->flags &ATA_QCFLAG_QUIET ; } -static int ata_eh_read_sense_success_non_ncq (struct ata_link *link) +static int ata_eh_get_non_ncq_success_sense (struct ata_link *link) {struct ata_port *ap = link->ap;struct ata_queued_cmd *qc;
@@ -2013,9 +2013,9 @@ static void ata_eh_get_success_sense(struct ata_link *link)* request sense ext command to retrieve the sense data. */ if (link->sactive) - ret =ata_eh_read_sense_success_ncq_log (link); + ret =ata_eh_get_ncq_success_sense (link);else - ret =ata_eh_read_sense_success_non_ncq (link); + ret =ata_eh_get_non_ncq_success_sense (link);if (ret) goto out;
drivers/ata/libata-sata.c +3 -4
@@ -1487,8 +1487,8 @@ static int ata_eh_read_log_10h(struct ata_device *dev, }/** -* ata_eh_read_sense_success_ncq_log - Read the sense data for successful -* NCQ commands log +* ata_eh_get_ncq_success_sense - Read and process the sense data for +* successful NCQ commands log page * @link: ATA link to get sense data for * * Read the sense data for successful NCQ commands log page to obtain
@@ -1501,7 +1501,7 @@ static int ata_eh_read_log_10h(struct ata_device *dev,* RETURNS: * 0 on success, -errno otherwise. */ -int ata_eh_read_sense_success_ncq_log (struct ata_link *link) +int ata_eh_get_ncq_success_sense (struct ata_link *link) {struct ata_device *dev = link->device;struct ata_port *ap = dev->link->ap;
@@ -1571,7 +1571,6 @@ int ata_eh_read_sense_success_ncq_log(struct ata_link *link)return ret; } -EXPORT_SYMBOL_GPL (ata_eh_read_sense_success_ncq_log);/** * ata_eh_analyze_ncq_error - analyze NCQ error
drivers/ata/libata.h +5 -0
@@ -95,11 +95,16 @@ extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,/* libata-sata.c */ #ifdef CONFIG_SATA_HOST int sata_down_spd_limit (struct ata_link *link,u32 spd_limit); +int ata_eh_get_ncq_success_sense (struct ata_link *link);#else static inline int sata_down_spd_limit (struct ata_link *link,u32 spd_limit) {return -EOPNOTSUPP ; } +static inline int ata_eh_get_ncq_success_sense (struct ata_link *link) +{ +return -EOPNOTSUPP ; +}#endif /* libata-acpi.c */
include/linux/libata.h +0 -5
@@ -1234,7 +1234,6 @@ extern int sata_link_hardreset(struct ata_link *link, bool *online, int (*check_ready)(struct ata_link *));extern int sata_link_resume (struct ata_link *link,const unsigned int *params,unsigned long deadline); -extern int ata_eh_read_sense_success_ncq_log (struct ata_link *link);extern void ata_eh_analyze_ncq_error (struct ata_link *link);#else static inline const unsigned int *
@@ -1277,10 +1276,6 @@ static inline int sata_link_resume(struct ata_link *link, {return -EOPNOTSUPP ; } -static inline int ata_eh_read_sense_success_ncq_log (struct ata_link *link) -{ -return -EOPNOTSUPP ; -}static inline void ata_eh_analyze_ncq_error (struct ata_link *link) { }#endif extern int sata_link_debounce(struct ata_link *link,
Signature · 1 line
2.46.0
Damien Le Moal
maintainerPATCH v3 6/7 ata: libata: Move sector_buf from struct ata_port to struct ata_device
In reply to Damien Le MoalThe 512B buffer sector_buf field of struct ata_port is used for scanning devices as well as during error recovery with ata EH. This buffer is thus useless if a port does not have a device connected to it. And also given that commands using this buffer are issued to devices, and not to ports, move this buffer definition from struct ata_port to struct ata_device.
This change slightly increases system memory usage for systems using a port-multiplier as in that case we do not need a per-device buffer for scanning devices (PMP does not allow parallel scanning) nor for EH (as when entering EH we are guaranteed that all commands to all devices connected to the PMP have completed or have been aborted). However, this change reduces memory usage on systems that have many ports with only few devices rives connected, which is a much more common use case than the PMP use case.
--- drivers/ata/libata-core.c | 63 ++++++++++++++++---------------------- drivers/ata/libata-eh.c | 2 +- drivers/ata/libata-pmp.c | 3 +- drivers/ata/libata-sata.c | 2 +- drivers/ata/libata-zpodd.c | 2 +- include/linux/libata.h | 4 ++- 6 files changed, 33 insertions(+), 43 deletions(-)
drivers/ata/libata-core.c +26 -37
@@ -2125,19 +2125,16 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,static int ata_log_supported (struct ata_device *dev,u8 log) { -struct ata_port *ap = dev->link->ap; -if (dev->quirks &ATA_QUIRK_NO_LOG_DIR )return 0 ; -if (ata_read_log_page (dev,ATA_LOG_DIRECTORY ,0 , ap->sector_buf,1 )) +if (ata_read_log_page (dev,ATA_LOG_DIRECTORY ,0 , dev->sector_buf,1 ))return 0 ; -return get_unaligned_le16 (&ap->sector_buf[log *2 ]); +return get_unaligned_le16 (&dev->sector_buf[log *2 ]); }static bool ata_identify_page_supported (struct ata_device *dev,u8 page) { -struct ata_port *ap = dev->link->ap;unsigned int err, i;if (dev->quirks &ATA_QUIRK_NO_ID_DEV_LOG )
@@ -2160,13 +2157,13 @@ static bool ata_identify_page_supported(struct ata_device *dev, u8 page)* Read IDENTIFY DEVICE data log, page 0, to figure out if the page is * supported. */ - err =ata_read_log_page (dev,ATA_LOG_IDENTIFY_DEVICE ,0 , ap->sector_buf, -1 ); + err =ata_read_log_page (dev,ATA_LOG_IDENTIFY_DEVICE ,0 , + dev->sector_buf,1 );if (err)return false; -for (i =0 ; i < ap->sector_buf[8 ]; i++) { -if (ap->sector_buf[9 + i] == page) +for (i =0 ; i < dev->sector_buf[8 ]; i++) { +if (dev->sector_buf[9 + i] == page)return true; }
@@ -2218,7 +2215,6 @@ static inline bool ata_dev_knobble(struct ata_device *dev)static void ata_dev_config_ncq_send_recv (struct ata_device *dev) { -struct ata_port *ap = dev->link->ap;unsigned int err_mask;if (!ata_log_supported (dev,ATA_LOG_NCQ_SEND_RECV )) {
@@ -2226,12 +2222,12 @@ static void ata_dev_config_ncq_send_recv(struct ata_device *dev)return ; } err_mask =ata_read_log_page (dev,ATA_LOG_NCQ_SEND_RECV , -0 , ap->sector_buf,1 ); +0 , dev->sector_buf,1 );if (!err_mask) {u8 *cmds = dev->ncq_send_recv_cmds; dev->flags |=ATA_DFLAG_NCQ_SEND_RECV ; -memcpy (cmds, ap->sector_buf,ATA_LOG_NCQ_SEND_RECV_SIZE ); +memcpy (cmds, dev->sector_buf,ATA_LOG_NCQ_SEND_RECV_SIZE );if (dev->quirks &ATA_QUIRK_NO_NCQ_TRIM ) {ata_dev_dbg (dev,"disabling queued TRIM support\n" );
@@ -2243,7 +2239,6 @@ static void ata_dev_config_ncq_send_recv(struct ata_device *dev)static void ata_dev_config_ncq_non_data (struct ata_device *dev) { -struct ata_port *ap = dev->link->ap;unsigned int err_mask;if (!ata_log_supported (dev,ATA_LOG_NCQ_NON_DATA )) {
@@ -2252,17 +2247,14 @@ static void ata_dev_config_ncq_non_data(struct ata_device *dev)return ; } err_mask =ata_read_log_page (dev,ATA_LOG_NCQ_NON_DATA , -0 , ap->sector_buf,1 ); -if (!err_mask) { -u8 *cmds = dev->ncq_non_data_cmds; - -memcpy (cmds, ap->sector_buf,ATA_LOG_NCQ_NON_DATA_SIZE ); - } +0 , dev->sector_buf,1 ); +if (!err_mask) +memcpy (dev->ncq_non_data_cmds, dev->sector_buf, +ATA_LOG_NCQ_NON_DATA_SIZE ); }static void ata_dev_config_ncq_prio (struct ata_device *dev) { -struct ata_port *ap = dev->link->ap;unsigned int err_mask;if (!ata_identify_page_supported (dev,ATA_LOG_SATA_SETTINGS ))
@@ -2271,12 +2263,11 @@ static void ata_dev_config_ncq_prio(struct ata_device *dev) err_mask =ata_read_log_page (dev,ATA_LOG_IDENTIFY_DEVICE ,ATA_LOG_SATA_SETTINGS , - ap->sector_buf, -1 ); + dev->sector_buf,1 );if (err_mask) goto not_supported; -if (!(ap->sector_buf[ATA_LOG_NCQ_PRIO_OFFSET ] &BIT (3 ))) +if (!(dev->sector_buf[ATA_LOG_NCQ_PRIO_OFFSET ] &BIT (3 ))) goto not_supported; dev->flags |=ATA_DFLAG_NCQ_PRIO ;
@@ -2392,9 +2383,8 @@ static void ata_dev_config_sense_reporting(struct ata_device *dev)static void ata_dev_config_zac (struct ata_device *dev) { -struct ata_port *ap = dev->link->ap;unsigned int err_mask; -u8 *identify_buf = ap->sector_buf; +u8 *identify_buf = dev->sector_buf; dev->zac_zones_optimal_open =U32_MAX ; dev->zac_zones_optimal_nonseq =U32_MAX ;
@@ -2446,7 +2436,6 @@ static void ata_dev_config_zac(struct ata_device *dev)static void ata_dev_config_trusted (struct ata_device *dev) { -struct ata_port *ap = dev->link->ap;u64 trusted_cap;unsigned int err;
@@ -2460,11 +2449,11 @@ static void ata_dev_config_trusted(struct ata_device *dev) } err =ata_read_log_page (dev,ATA_LOG_IDENTIFY_DEVICE ,ATA_LOG_SECURITY , - ap->sector_buf,1 ); + dev->sector_buf,1 );if (err)return ; - trusted_cap =get_unaligned_le64 (&ap->sector_buf[40 ]); + trusted_cap =get_unaligned_le64 (&dev->sector_buf[40 ]);if (!(trusted_cap & (1ULL <<63 ))) {ata_dev_dbg (dev,"Trusted Computing capability qword not valid!\n" );
@@ -2492,12 +2481,12 @@ static void ata_dev_config_cdl(struct ata_device *dev) err_mask =ata_read_log_page (dev,ATA_LOG_IDENTIFY_DEVICE ,ATA_LOG_SUPPORTED_CAPABILITIES , - ap->sector_buf,1 ); + dev->sector_buf,1 );if (err_mask) goto not_supported;/* Check Command Duration Limit Supported bits */ - val =get_unaligned_le64 (&ap->sector_buf[168 ]); + val =get_unaligned_le64 (&dev->sector_buf[168 ]);if (!(val &BIT_ULL (63 )) || !(val &BIT_ULL (0 ))) goto not_supported;
@@ -2510,7 +2499,7 @@ static void ata_dev_config_cdl(struct ata_device *dev)* We must have support for the sense data for successful NCQ commands * log indicated by the successful NCQ command sense data supported bit. */ - val =get_unaligned_le64 (&ap->sector_buf[8 ]); + val =get_unaligned_le64 (&dev->sector_buf[8 ]);if (!(val &BIT_ULL (63 )) || !(val &BIT_ULL (47 ))) {ata_dev_warn (dev,"CDL supported but Successful NCQ Command Sense Data is not supported\n" );
@@ -2530,11 +2519,11 @@ static void ata_dev_config_cdl(struct ata_device *dev)*/ err_mask =ata_read_log_page (dev,ATA_LOG_IDENTIFY_DEVICE ,ATA_LOG_CURRENT_SETTINGS , - ap->sector_buf,1 ); + dev->sector_buf,1 );if (err_mask) goto not_supported; - val =get_unaligned_le64 (&ap->sector_buf[8 ]); + val =get_unaligned_le64 (&dev->sector_buf[8 ]); cdl_enabled = val &BIT_ULL (63 ) && val &BIT_ULL (21 );if (dev->flags &ATA_DFLAG_CDL_ENABLED ) {if (!cdl_enabled) {
@@ -2591,13 +2580,13 @@ static void ata_dev_config_cdl(struct ata_device *dev)* Command duration limits is supported: cache the CDL log page 18h * (command duration descriptors). */ - err_mask =ata_read_log_page (dev,ATA_LOG_CDL ,0 , ap->sector_buf,1 ); + err_mask =ata_read_log_page (dev,ATA_LOG_CDL ,0 , dev->sector_buf,1 );if (err_mask) {ata_dev_warn (dev,"Read Command Duration Limits log failed\n" ); goto not_supported; } -memcpy (dev->cdl, ap->sector_buf,ATA_LOG_CDL_SIZE ); +memcpy (dev->cdl, dev->sector_buf,ATA_LOG_CDL_SIZE ); dev->flags |=ATA_DFLAG_CDL ;return ;
@@ -2689,7 +2678,7 @@ static void ata_dev_config_fua(struct ata_device *dev)static void ata_dev_config_devslp (struct ata_device *dev) { -u8 *sata_setting = dev->link->ap->sector_buf; +u8 *sata_setting = dev->sector_buf;unsigned int err_mask;int i, j;
@@ -3759,7 +3748,7 @@ static int ata_dev_same_device(struct ata_device *dev, unsigned int new_class,int ata_dev_reread_id (struct ata_device *dev,unsigned int readid_flags) {unsigned int class = dev->class; -u16 *id = (void *)dev->link->ap->sector_buf; +u16 *id = (void *)dev->sector_buf;int rc;/* read ID data */
drivers/ata/libata-eh.c +1 -1
@@ -3284,7 +3284,7 @@ static int atapi_eh_clear_ua(struct ata_device *dev)int i;for (i =0 ; i <ATA_EH_UA_TRIES ; i++) { -u8 *sense_buffer = dev->link->ap->sector_buf; +u8 *sense_buffer = dev->sector_buf;u8 sense_key =0 ;unsigned int err_mask;
drivers/ata/libata-pmp.c +1 -2
@@ -648,8 +648,7 @@ static int sata_pmp_same_pmp(struct ata_device *dev, const u32 *new_gscr)static int sata_pmp_revalidate (struct ata_device *dev,unsigned int new_class) {struct ata_link *link = dev->link; -struct ata_port *ap = link->ap; -u32 *gscr = (void *)ap->sector_buf; +u32 *gscr = (void *)dev->sector_buf;int rc;ata_eh_about_to_do (link,NULL ,ATA_EH_REVALIDATE );
drivers/ata/libata-sata.c +1 -1
@@ -1448,7 +1448,7 @@ EXPORT_SYMBOL_GPL(sata_async_notification);static int ata_eh_read_log_10h (struct ata_device *dev,int *tag,struct ata_taskfile *tf) { -u8 *buf = dev->link->ap->sector_buf; +u8 *buf = dev->sector_buf;unsigned int err_mask;u8 csum;int i;
drivers/ata/libata-zpodd.c +1 -1
@@ -112,7 +112,7 @@ static bool zpready(struct ata_device *dev)if (!ret || sense_key !=NOT_READY )return false; - sense_buf = dev->link->ap->sector_buf; + sense_buf = dev->sector_buf; ret =atapi_eh_request_sense (dev, sense_buf, sense_key);if (ret)return false;
include/linux/libata.h +3 -1
@@ -769,6 +769,9 @@ struct ata_device {int spdn_cnt;/* ering is CLEAR_END, read comment above CLEAR_END */ struct ata_ering ering; + +/* For EH */ +u8 sector_buf[ATA_SECT_SIZE ]____cacheline_aligned ; };/* Fields between ATA_DEVICE_CLEAR_BEGIN and ATA_DEVICE_CLEAR_END are
@@ -916,7 +919,6 @@ struct ata_port {#endif /* owned by EH */ u8 *ncq_sense_buf; -u8 sector_buf[ATA_SECT_SIZE ]____cacheline_aligned ; };/* The following initializer overrides a method to NULL whether one of
Signature · 1 line
2.46.0
Re: [PATCH v3 6/7] ata: libata: Move sector_buf from struct ata_port to struct ata_device
Quoted 16 lines · Damien Le Moal
The 512B buffer
sector_buffield of structata_portis used for scanning devices as well as during error recovery with ata EH. This buffer is thus useless if a port does not have a device connected to it. And also given that commands using this buffer are issued to devices, and not to ports, move this buffer definition from structata_portto structata_device.This change slightly increases system memory usage for systems using a port-multiplier as in that case we do not need a per-device buffer for scanning devices (PMP does not allow parallel scanning) nor for EH (as when entering EH we are guaranteed that all commands to all devices connected to the PMP have completed or have been aborted). However, this change reduces memory usage on systems that have many ports with only few devices rives connected, which is a much more common use case than the PMP use case.
Quoted 3 lines
Signed-off-by Damien Le Moal <[email protected]>Reviewed-by Hannes Reinecke <[email protected]>---
...and since I suggested this specific change in my reply to your V1 series, I feel that I should get a:
:)
Damien Le Moal
maintainerRe: [PATCH v3 6/7] ata: libata: Move sector_buf from struct ata_port to struct ata_device
Quoted 21 lines · Niklas Cassel
Quoted 16 lines · Damien Le Moal
The 512B buffer
sector_buffield of structata_portis used for scanning devices as well as during error recovery with ata EH. This buffer is thus useless if a port does not have a device connected to it. And also given that commands using this buffer are issued to devices, and not to ports, move this buffer definition from structata_portto structata_device.This change slightly increases system memory usage for systems using a port-multiplier as in that case we do not need a per-device buffer for scanning devices (PMP does not allow parallel scanning) nor for EH (as when entering EH we are guaranteed that all commands to all devices connected to the PMP have completed or have been aborted). However, this change reduces memory usage on systems that have many ports with only few devices rives connected, which is a much more common use case than the PMP use case.
Quoted 3 lines
Signed-off-by Damien Le Moal <[email protected]>Reviewed-by Hannes Reinecke <[email protected]>---
Quoted 5 lines
Reviewed-by Niklas Cassel <[email protected]>...and since I suggested this specific change in my reply to your V1 series, I feel that I should get a:
Suggested-by Niklas Cassel <[email protected]>
Indeed. Added.
Quoted 2 lines
:)
Signature · 2 lines
Damien Le Moal Western Digital Research
Damien Le Moal
maintainerPATCH v3 7/7 ata: libata: Improve CDL resource management
In reply to Damien Le MoalThe ncq_sense_buf buffer field of struct ata_port is allocated and used only for devices that support the Command Duration Limits (CDL) feature. However, the cdl buffer of struct ata_device, which is used to cache the command duration limits log page for devices supporting CDL is always allocated as part of struct ata_device, which is wasteful of memory for devices that do not support this feature.
Clean this up by defining both buffers as part of the new ata_cdl structure and allocating this structure only for devices that support the CDL feature. This new structure is attached to struct ata_device using the cdl pointer.
The functions ata_dev_init_cdl_resources() and ata_dev_cleanup_cdl_resources() are defined to manage this new structure allocation, initialization and cleanup when a port is removed or a device disabled. ata_dev_init_cdl_resources() is called from ata_dev_config_cdl() only for devices that support CDL. ata_dev_cleanup_cdl_resources() is called from ata_port_free() and ata_eh_dev_disable() to free the ata_cdl structure when a device is being disabled by EH or its port being removed.
Note that the name of the former cdl log buffer of struct ata_device is changed to desc_log_buf to make it clearer that it is a buffer for the limit descriptors log page.
This change reduces the size of struct ata_device, thus reducing memory usage for ATA devices that do not support the CDL feature.
--- drivers/ata/libata-core.c | 74 ++++++++++++++++++++++++++------------- drivers/ata/libata-eh.c | 2 ++ drivers/ata/libata-sata.c | 2 +- drivers/ata/libata-scsi.c | 2 +- drivers/ata/libata.h | 1 + include/linux/libata.h | 21 ++++++++--- 6 files changed, 72 insertions(+), 30 deletions(-)
drivers/ata/libata-core.c +50 -24
@@ -2464,12 +2464,40 @@ static void ata_dev_config_trusted(struct ata_device *dev) dev->flags |=ATA_DFLAG_TRUSTED ; } +static int ata_dev_init_cdl_resources (struct ata_device *dev) +{ +struct ata_cdl *cdl = dev->cdl; +unsigned int err_mask; + +if (!cdl) { + cdl =kzalloc (sizeof (struct ata_cdl ),GFP_KERNEL ); +if (!cdl) +return -ENOMEM ; + dev->cdl = cdl; + } + + err_mask =ata_read_log_page (dev,ATA_LOG_CDL ,0 , cdl->desc_log_buf, +ATA_LOG_CDL_SIZE /ATA_SECT_SIZE ); +if (err_mask) { +ata_dev_warn (dev,"Read Command Duration Limits log failed\n" ); +return -EIO ; + } + +return 0 ; +} + +void ata_dev_cleanup_cdl_resources (struct ata_device *dev) +{ +kfree (dev->cdl); + dev->cdl =NULL ; +} +static void ata_dev_config_cdl (struct ata_device *dev) { -struct ata_port *ap = dev->link->ap;unsigned int err_mask;bool cdl_enabled;u64 val; +int ret;if (ata_id_major_version (dev->id) <11 ) goto not_supported;
@@ -2564,37 +2592,20 @@ static void ata_dev_config_cdl(struct ata_device *dev) } } -/* -* Allocate a buffer to handle reading the sense data for successful -* NCQ Commands log page for commands using a CDL with one of the limit -* policy set to 0xD (successful completion with sense data available -* bit set). -*/ -if (!ap->ncq_sense_buf) { - ap->ncq_sense_buf =kmalloc (ATA_LOG_SENSE_NCQ_SIZE ,GFP_KERNEL ); -if (!ap->ncq_sense_buf) - goto not_supported; - } - -/* -* Command duration limits is supported: cache the CDL log page 18h -* (command duration descriptors). -*/ - err_mask =ata_read_log_page (dev,ATA_LOG_CDL ,0 , dev->sector_buf,1 ); -if (err_mask) { -ata_dev_warn (dev,"Read Command Duration Limits log failed\n" ); +/* CDL is supported: allocate and initialize needed resources. */ + ret =ata_dev_init_cdl_resources (dev); +if (ret) { +ata_dev_warn (dev,"Initialize CDL resources failed\n" ); goto not_supported; } -memcpy (dev->cdl, dev->sector_buf,ATA_LOG_CDL_SIZE ); dev->flags |=ATA_DFLAG_CDL ;return ; not_supported: dev->flags &= ~(ATA_DFLAG_CDL |ATA_DFLAG_CDL_ENABLED ); -kfree (ap->ncq_sense_buf); - ap->ncq_sense_buf =NULL ; +ata_dev_cleanup_cdl_resources (dev); }static int ata_dev_config_lba (struct ata_device *dev)
@@ -5446,12 +5457,19 @@ EXPORT_SYMBOL_GPL(ata_port_alloc);void ata_port_free (struct ata_port *ap) { +struct ata_device *dev; +struct ata_link *link; +if (!ap)return ; +ata_for_each_link (link, ap,HOST_FIRST ) { +ata_for_each_dev (dev, link,ALL ) +ata_dev_cleanup_cdl_resources (dev); + } +kfree (ap->pmp_link);kfree (ap->slave_link); -kfree (ap->ncq_sense_buf);ida_free (&ata_ida, ap->print_id);kfree (ap); }
@@ -6000,11 +6018,19 @@ static void ata_port_detach(struct ata_port *ap)ata_port_wait_eh (ap);mutex_lock (&ap->scsi_scan_mutex); + +/* Cleanup CDL device resources */ +ata_for_each_link (link, ap,HOST_FIRST ) { +ata_for_each_dev (dev, link,ALL ) +ata_dev_cleanup_cdl_resources (dev); + } +spin_lock_irqsave (ap->lock, flags);/* Remove scsi devices */ ata_for_each_link (link, ap,HOST_FIRST ) {ata_for_each_dev (dev, link,ALL ) { +ata_dev_cleanup_cdl_resources (dev);if (dev->sdev) {spin_unlock_irqrestore (ap->lock, flags);scsi_remove_device (dev->sdev);
drivers/ata/libata-eh.c +2 -0
@@ -500,6 +500,8 @@ static void ata_eh_dev_disable(struct ata_device *dev)ata_down_xfermask_limit (dev,ATA_DNXFER_FORCE_PIO0 |ATA_DNXFER_QUIET ); dev->class++; +ata_dev_cleanup_cdl_resources (dev); +/* From now till the next successful probe, ering is used to * track probe failures. Clear accumulated device error info. */
drivers/ata/libata-sata.c +1 -1
@@ -1505,7 +1505,7 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link) {struct ata_device *dev = link->device;struct ata_port *ap = dev->link->ap; -u8 *buf = ap->ncq_sense_buf; +u8 *buf = dev->cdl->ncq_sense_log_buf;struct ata_queued_cmd *qc;unsigned int err_mask, tag;u8 *sense, sk =0 , asc =0 , ascq =0 ;
drivers/ata/libata-scsi.c +1 -1
@@ -2259,7 +2259,7 @@ static inline u16 ata_xlat_cdl_limit(u8 *buf)static unsigned int ata_msense_control_spgt2 (struct ata_device *dev,u8 *buf,u8 spg) { -u8 *b, *cdl = dev->cdl, *desc; +u8 *b, *cdl = dev->cdl->desc_log_buf, *desc;u32 policy;int i;
drivers/ata/libata.h +1 -0
@@ -89,6 +89,7 @@ extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg);extern const char *sata_spd_string (unsigned int spd);extern unsigned int ata_read_log_page (struct ata_device *dev,u8 log,u8 page,void *buf,unsigned int sectors); +void ata_dev_cleanup_cdl_resources (struct ata_device *dev);#define to_ata_port (d) container_of(d, struct ata_port, tdev)
include/linux/libata.h +17 -4
@@ -700,6 +700,21 @@ struct ata_cpr_log { struct ata_cpr cpr[] __counted_by(nr_cpr); }; +struct ata_cdl { +/* +* Buffer to cache the CDL log page 18h (command duration descriptors) +* for SCSI-ATA translation. +*/ +u8 desc_log_buf[ATA_LOG_CDL_SIZE ]; + +/* +* Buffer to handle reading the sense data for successful NCQ Commands +* log page for commands using a CDL with one of the limits policy set +* to 0xD (successful completion with sense data available bit set). +*/ +u8 ncq_sense_log_buf[ATA_LOG_SENSE_NCQ_SIZE ]; +}; +struct ata_device {struct ata_link *link;unsigned int devno;/* 0 or 1 */
@@ -762,8 +777,8 @@ struct ata_device {/* Concurrent positioning ranges */ struct ata_cpr_log *cpr_log; -/* Command Duration Limits log support */ -u8 cdl[ATA_LOG_CDL_SIZE ]; +/* Command Duration Limits support */ +struct ata_cdl *cdl;/* error history */ int spdn_cnt;
@@ -917,8 +932,6 @@ struct ata_port {#ifdef CONFIG_ATA_ACPI struct ata_acpi_gtm __acpi_init_gtm;/* use ata_acpi_init_gtm() */ #endif -/* owned by EH */ -u8 *ncq_sense_buf; };/* The following initializer overrides a method to NULL whether one of
Signature · 1 line
2.46.0
Niklas Cassel
maintainerRe: [PATCH v3 7/7] ata: libata: Improve CDL resource management
Quoted 44 lines · Damien Le Moal
The
ncq_sense_bufbuffer field of structata_portis allocated and used only for devices that support the Command Duration Limits (CDL) feature. However, the cdl buffer of structata_device, which is used to cache the command duration limits log page for devices supporting CDL is always allocated as part of structata_device, which is wasteful of memory for devices that do not support this feature.Clean this up by defining both buffers as part of the new
ata_cdlstructure and allocating this structure only for devices that support the CDL feature. This new structure is attached to structata_deviceusing the cdl pointer.The functions
ata_dev_init_cdl_resources()andata_dev_cleanup_cdl_resources()are defined to manage this new structure allocation, initialization and cleanup when a port is removed or a device disabled.ata_dev_init_cdl_resources()is called fromata_dev_config_cdl()only for devices that support CDL.ata_dev_cleanup_cdl_resources()is called fromata_port_free()andata_eh_dev_disable()to free theata_cdlstructure when a device is being disabled by EH or its port being removed.Note that the name of the former cdl log buffer of struct
ata_deviceis changed todesc_log_bufto make it clearer that it is a buffer for the limit descriptors log page.This change reduces the size of struct
ata_device, thus reducing memory usage for ATA devices that do not support the CDL feature.Signed-off-by Damien Le Moal <[email protected]>Reviewed-by Hannes Reinecke <[email protected]>--- drivers/ata/libata-core.c | 74 ++++++++++++++++++++++++++------------- drivers/ata/libata-eh.c | 2 ++ drivers/ata/libata-sata.c | 2 +- drivers/ata/libata-scsi.c | 2 +- drivers/ata/libata.h | 1 + include/linux/libata.h | 21 ++++++++--- 6 files changed, 72 insertions(+), 30 deletions(-)drivers/ata/libata-core.c +0
-0
drivers/ata/libata-core.c static void ata_dev_config_trusted(struct ata_device *dev) · line 2473
Quoted 10 lines
drivers/ata/libata-core.c +7
-0@@ -2464,12 +2464,40 @@ static void ata_dev_config_trusted(struct ata_device *dev) dev->flags |=ATA_DFLAG_TRUSTED ; } +static int ata_dev_init_cdl_resources (struct ata_device *dev) +{ +struct ata_cdl *cdl = dev->cdl; +unsigned int err_mask; + +if (!cdl) { + cdl =kzalloc (sizeof (struct ata_cdl ),GFP_KERNEL );
cdl = kzalloc(sizeof(*ata_cdl), GFP_KERNEL);
is more robust, as with the current code, if someone changes type of *cdl, there is a chance to introduce a bug if they forget to update this line.
Quoted 78 lines
drivers/ata/libata-core.c +27
-23+if (!cdl) +return -ENOMEM ; + dev->cdl = cdl; + } + + err_mask =ata_read_log_page (dev,ATA_LOG_CDL ,0 , cdl->desc_log_buf, +ATA_LOG_CDL_SIZE /ATA_SECT_SIZE ); +if (err_mask) { +ata_dev_warn (dev,"Read Command Duration Limits log failed\n" ); +return -EIO ; + } + +return 0 ; +} + +void ata_dev_cleanup_cdl_resources (struct ata_device *dev) +{ +kfree (dev->cdl); + dev->cdl =NULL ; +} +static void ata_dev_config_cdl (struct ata_device *dev) { -struct ata_port *ap = dev->link->ap;unsigned int err_mask;bool cdl_enabled;u64 val; +int ret;if (ata_id_major_version (dev->id) <11 ) goto not_supported;@@ -2564,37 +2592,20 @@ static void ata_dev_config_cdl(struct ata_device *dev) } } -/* -* Allocate a buffer to handle reading the sense data for successful -* NCQ Commands log page for commands using a CDL with one of the limit -* policy set to 0xD (successful completion with sense data available -* bit set). -*/ -if (!ap->ncq_sense_buf) { - ap->ncq_sense_buf =kmalloc (ATA_LOG_SENSE_NCQ_SIZE ,GFP_KERNEL ); -if (!ap->ncq_sense_buf) - goto not_supported; - } - -/* -* Command duration limits is supported: cache the CDL log page 18h -* (command duration descriptors). -*/ - err_mask =ata_read_log_page (dev,ATA_LOG_CDL ,0 , dev->sector_buf,1 ); -if (err_mask) { -ata_dev_warn (dev,"Read Command Duration Limits log failed\n" ); +/* CDL is supported: allocate and initialize needed resources. */ + ret =ata_dev_init_cdl_resources (dev); +if (ret) { +ata_dev_warn (dev,"Initialize CDL resources failed\n" ); goto not_supported; } -memcpy (dev->cdl, dev->sector_buf,ATA_LOG_CDL_SIZE ); dev->flags |=ATA_DFLAG_CDL ;return ; not_supported: dev->flags &= ~(ATA_DFLAG_CDL |ATA_DFLAG_CDL_ENABLED ); -kfree (ap->ncq_sense_buf); - ap->ncq_sense_buf =NULL ; +ata_dev_cleanup_cdl_resources (dev); }static int ata_dev_config_lba (struct ata_device *dev)@@ -5446,12 +5457,19 @@ EXPORT_SYMBOL_GPL(ata_port_alloc);void ata_port_free (struct ata_port *ap) {
drivers/ata/libata-core.c EXPORT_SYMBOL_GPL(ata_port_alloc); · line 5469
Quoted 10 lines
drivers/ata/libata-core.c +7
-0+struct ata_device *dev; +struct ata_link *link; +if (!ap)return ; +ata_for_each_link (link, ap,HOST_FIRST ) { +ata_for_each_dev (dev, link,ALL ) +ata_dev_cleanup_cdl_resources (dev); + }
Here you free the resources when the port is freed, nice.
Quoted 8 lines
drivers/ata/libata-core.c +1
-1+kfree (ap->pmp_link);kfree (ap->slave_link); -kfree (ap->ncq_sense_buf);ida_free (&ata_ida, ap->print_id);kfree (ap); }
drivers/ata/libata-core.c static void ata_port_detach(struct ata_port *ap) · line 6025
Quoted 8 lines
drivers/ata/libata-core.c +5
-0@@ -6000,11 +6018,19 @@ static void ata_port_detach(struct ata_port *ap)ata_port_wait_eh (ap);mutex_lock (&ap->scsi_scan_mutex); + +/* Cleanup CDL device resources */ +ata_for_each_link (link, ap,HOST_FIRST ) { +ata_for_each_dev (dev, link,ALL ) +ata_dev_cleanup_cdl_resources (dev);
Here you clean up resources.
Why?
Resources will get cleaned up when ata_port_free() is called, which will be called by ata_devres_release() -> ata_host_put() -> ata_host_release() -> ata_port_free(), when the device is removed.
I don't see any reason to free it here as well.
That will just ensure that the ata_dev_cleanup_cdl_resources() call by ata_port_free() will do nothing.
drivers/ata/libata-core.c static void ata_port_detach(struct ata_port *ap) · line 6033
Quoted 8 lines
drivers/ata/libata-core.c +3
-0+ } +spin_lock_irqsave (ap->lock, flags);/* Remove scsi devices */ ata_for_each_link (link, ap,HOST_FIRST ) {ata_for_each_dev (dev, link,ALL ) { +ata_dev_cleanup_cdl_resources (dev);
What is the point of this call?
You already looped through all devices in this same function (ata_port_detach()) just above.
But just like my comment above, I don't see any reason to call ata_dev_cleanup_cdl_resources() here. The resources will get freed by ata_port_free() when the device is removed.
I think you can remove both ata_dev_cleanup_cdl_resources() calls in ata_port_detach().
Quoted 103 lines
drivers/ata/libata-core.c +0
-0if (dev->sdev) {spin_unlock_irqrestore (ap->lock, flags);scsi_remove_device (dev->sdev);drivers/ata/libata-eh.c +2
-0@@ -500,6 +500,8 @@ static void ata_eh_dev_disable(struct ata_device *dev)ata_down_xfermask_limit (dev,ATA_DNXFER_FORCE_PIO0 |ATA_DNXFER_QUIET ); dev->class++; +ata_dev_cleanup_cdl_resources (dev); +/* From now till the next successful probe, ering is used to * track probe failures. Clear accumulated device error info. */ drivers/ata/libata-sata.c +1
-1@@ -1505,7 +1505,7 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link) {struct ata_device *dev = link->device;struct ata_port *ap = dev->link->ap; -u8 *buf = ap->ncq_sense_buf; +u8 *buf = dev->cdl->ncq_sense_log_buf;struct ata_queued_cmd *qc;unsigned int err_mask, tag;u8 *sense, sk =0 , asc =0 , ascq =0 ;drivers/ata/libata-scsi.c +1
-1@@ -2259,7 +2259,7 @@ static inline u16 ata_xlat_cdl_limit(u8 *buf)static unsigned int ata_msense_control_spgt2 (struct ata_device *dev,u8 *buf,u8 spg) { -u8 *b, *cdl = dev->cdl, *desc; +u8 *b, *cdl = dev->cdl->desc_log_buf, *desc;u32 policy;int i;drivers/ata/libata.h +1
-0@@ -89,6 +89,7 @@ extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg);extern const char *sata_spd_string (unsigned int spd);extern unsigned int ata_read_log_page (struct ata_device *dev,u8 log,u8 page,void *buf,unsigned int sectors); +void ata_dev_cleanup_cdl_resources (struct ata_device *dev);#define to_ata_port (d) container_of(d, struct ata_port, tdev)include/linux/libata.h +17
-4@@ -700,6 +700,21 @@ struct ata_cpr_log { struct ata_cpr cpr[] __counted_by(nr_cpr); }; +struct ata_cdl { +/* +* Buffer to cache the CDL log page 18h (command duration descriptors) +* for SCSI-ATA translation. +*/ +u8 desc_log_buf[ATA_LOG_CDL_SIZE ]; + +/* +* Buffer to handle reading the sense data for successful NCQ Commands +* log page for commands using a CDL with one of the limits policy set +* to 0xD (successful completion with sense data available bit set). +*/ +u8 ncq_sense_log_buf[ATA_LOG_SENSE_NCQ_SIZE ]; +}; +struct ata_device {struct ata_link *link;unsigned int devno;/* 0 or 1 */ @@ -762,8 +777,8 @@ struct ata_device {/* Concurrent positioning ranges */ struct ata_cpr_log *cpr_log; -/* Command Duration Limits log support */ -u8 cdl[ATA_LOG_CDL_SIZE ]; +/* Command Duration Limits support */ +struct ata_cdl *cdl;/* error history */ int spdn_cnt;@@ -917,8 +932,6 @@ struct ata_port {#ifdef CONFIG_ATA_ACPI struct ata_acpi_gtm __acpi_init_gtm;/* use ata_acpi_init_gtm() */ #endif -/* owned by EH */ -u8 *ncq_sense_buf; };/* The following initializer overrides a method to NULL whether one of Signature · 1 line
2.46.0
Damien Le Moal
maintainerRe: [PATCH v3 7/7] ata: libata: Improve CDL resource management
Quoted 164 lines · Niklas Cassel
Quoted 44 lines · Damien Le Moal
The
ncq_sense_bufbuffer field of structata_portis allocated and used only for devices that support the Command Duration Limits (CDL) feature. However, the cdl buffer of structata_device, which is used to cache the command duration limits log page for devices supporting CDL is always allocated as part of structata_device, which is wasteful of memory for devices that do not support this feature.Clean this up by defining both buffers as part of the new
ata_cdlstructure and allocating this structure only for devices that support the CDL feature. This new structure is attached to structata_deviceusing the cdl pointer.The functions
ata_dev_init_cdl_resources()andata_dev_cleanup_cdl_resources()are defined to manage this new structure allocation, initialization and cleanup when a port is removed or a device disabled.ata_dev_init_cdl_resources()is called fromata_dev_config_cdl()only for devices that support CDL.ata_dev_cleanup_cdl_resources()is called fromata_port_free()andata_eh_dev_disable()to free theata_cdlstructure when a device is being disabled by EH or its port being removed.Note that the name of the former cdl log buffer of struct
ata_deviceis changed todesc_log_bufto make it clearer that it is a buffer for the limit descriptors log page.This change reduces the size of struct
ata_device, thus reducing memory usage for ATA devices that do not support the CDL feature.Signed-off-by Damien Le Moal <[email protected]>Reviewed-by Hannes Reinecke <[email protected]>--- drivers/ata/libata-core.c | 74 ++++++++++++++++++++++++++------------- drivers/ata/libata-eh.c | 2 ++ drivers/ata/libata-sata.c | 2 +- drivers/ata/libata-scsi.c | 2 +- drivers/ata/libata.h | 1 + include/linux/libata.h | 21 ++++++++--- 6 files changed, 72 insertions(+), 30 deletions(-)drivers/ata/libata-core.c +0
-0drivers/ata/libata-core.c static void ata_dev_config_trusted(struct ata_device *dev) · line 2473
Quoted 10 lines
drivers/ata/libata-core.c +7
-0@@ -2464,12 +2464,40 @@ static void ata_dev_config_trusted(struct ata_device *dev) dev->flags |=ATA_DFLAG_TRUSTED ; } +static int ata_dev_init_cdl_resources (struct ata_device *dev) +{ +struct ata_cdl *cdl = dev->cdl; +unsigned int err_mask; + +if (!cdl) { + cdl =kzalloc (sizeof (struct ata_cdl ),GFP_KERNEL );cdl =
kzalloc(sizeof(*ata_cdl), GFP_KERNEL);is more robust, as with the current code, if someone changes type of *cdl, there is a chance to introduce a bug if they forget to update this line.
Quoted 78 lines
drivers/ata/libata-core.c +27
-23+if (!cdl) +return -ENOMEM ; + dev->cdl = cdl; + } + + err_mask =ata_read_log_page (dev,ATA_LOG_CDL ,0 , cdl->desc_log_buf, +ATA_LOG_CDL_SIZE /ATA_SECT_SIZE ); +if (err_mask) { +ata_dev_warn (dev,"Read Command Duration Limits log failed\n" ); +return -EIO ; + } + +return 0 ; +} + +void ata_dev_cleanup_cdl_resources (struct ata_device *dev) +{ +kfree (dev->cdl); + dev->cdl =NULL ; +} +static void ata_dev_config_cdl (struct ata_device *dev) { -struct ata_port *ap = dev->link->ap;unsigned int err_mask;bool cdl_enabled;u64 val; +int ret;if (ata_id_major_version (dev->id) <11 ) goto not_supported;@@ -2564,37 +2592,20 @@ static void ata_dev_config_cdl(struct ata_device *dev) } } -/* -* Allocate a buffer to handle reading the sense data for successful -* NCQ Commands log page for commands using a CDL with one of the limit -* policy set to 0xD (successful completion with sense data available -* bit set). -*/ -if (!ap->ncq_sense_buf) { - ap->ncq_sense_buf =kmalloc (ATA_LOG_SENSE_NCQ_SIZE ,GFP_KERNEL ); -if (!ap->ncq_sense_buf) - goto not_supported; - } - -/* -* Command duration limits is supported: cache the CDL log page 18h -* (command duration descriptors). -*/ - err_mask =ata_read_log_page (dev,ATA_LOG_CDL ,0 , dev->sector_buf,1 ); -if (err_mask) { -ata_dev_warn (dev,"Read Command Duration Limits log failed\n" ); +/* CDL is supported: allocate and initialize needed resources. */ + ret =ata_dev_init_cdl_resources (dev); +if (ret) { +ata_dev_warn (dev,"Initialize CDL resources failed\n" ); goto not_supported; } -memcpy (dev->cdl, dev->sector_buf,ATA_LOG_CDL_SIZE ); dev->flags |=ATA_DFLAG_CDL ;return ; not_supported: dev->flags &= ~(ATA_DFLAG_CDL |ATA_DFLAG_CDL_ENABLED ); -kfree (ap->ncq_sense_buf); - ap->ncq_sense_buf =NULL ; +ata_dev_cleanup_cdl_resources (dev); }static int ata_dev_config_lba (struct ata_device *dev)@@ -5446,12 +5457,19 @@ EXPORT_SYMBOL_GPL(ata_port_alloc);void ata_port_free (struct ata_port *ap) {drivers/ata/libata-core.c EXPORT_SYMBOL_GPL(ata_port_alloc); · line 5469
Quoted 10 lines
drivers/ata/libata-core.c +7
-0+struct ata_device *dev; +struct ata_link *link; +if (!ap)return ; +ata_for_each_link (link, ap,HOST_FIRST ) { +ata_for_each_dev (dev, link,ALL ) +ata_dev_cleanup_cdl_resources (dev); + }Here you free the resources when the port is freed, nice.
Quoted 10 lines
drivers/ata/libata-core.c +1
-1+kfree (ap->pmp_link);kfree (ap->slave_link); -kfree (ap->ncq_sense_buf);ida_free (&ata_ida, ap->print_id);kfree (ap); }@@ -6000,11 +6018,19 @@ static void ata_port_detach(struct ata_port *ap)ata_port_wait_eh (ap);
Quoted 13 lines
drivers/ata/libata-core.c static void ata_port_detach(struct ata_port *ap) · line 6025
Quoted 6 lines
drivers/ata/libata-core.c +5
-0mutex_lock (&ap->scsi_scan_mutex); + +/* Cleanup CDL device resources */ +ata_for_each_link (link, ap,HOST_FIRST ) { +ata_for_each_dev (dev, link,ALL ) +ata_dev_cleanup_cdl_resources (dev);Here you clean up resources.
Why?
Resources will get cleaned up whenata_port_free()is called, which will be called byata_devres_release()->ata_host_put()->ata_host_release()->ata_port_free(), when the device is removed.
That happens only if the host (=port) is removed, but not if only the device is being removed, e.g. because it was yanked out of its slot (hotplug) or the user removed it using sysfs. In such case, the port remains and is not deleted, so ata_port_free() is not called. Ports exists for as long as the adapter exist.
Quoted 1 line
I don't see any reason to free it here as well.
Hotplug of drives :)
Quoted 3 lines
That will just ensure that the
ata_dev_cleanup_cdl_resources()call byata_port_free()will do nothing.
Yes, if the device is removed before the port, ata_dev_cleanup_cdl_resources() becomes a no-op.
Quoted 6 lines
Quoted 6 lines
drivers/ata/libata-core.c +2
-0+ } +spin_lock_irqsave (ap->lock, flags);
Quoted 6 lines
drivers/ata/libata-core.c static void ata_port_detach(struct ata_port *ap) · line 6033
Quoted 4 lines
drivers/ata/libata-core.c +1
-0/* Remove scsi devices */ ata_for_each_link (link, ap,HOST_FIRST ) {ata_for_each_dev (dev, link,ALL ) { +ata_dev_cleanup_cdl_resources (dev);What is the point of this call?
That is a mistake, should not be here. Will remove it.
Quoted 114 lines
You already looped through all devices in this same function (
ata_port_detach()) just above.But just like my comment above, I don't see any reason to call
ata_dev_cleanup_cdl_resources()here. The resources will get freed byata_port_free()when the device is removed.I think you can remove both
ata_dev_cleanup_cdl_resources()calls inata_port_detach().Quoted 103 lines
drivers/ata/libata-core.c +0
-0if (dev->sdev) {spin_unlock_irqrestore (ap->lock, flags);scsi_remove_device (dev->sdev);drivers/ata/libata-eh.c +2
-0@@ -500,6 +500,8 @@ static void ata_eh_dev_disable(struct ata_device *dev)ata_down_xfermask_limit (dev,ATA_DNXFER_FORCE_PIO0 |ATA_DNXFER_QUIET ); dev->class++; +ata_dev_cleanup_cdl_resources (dev); +/* From now till the next successful probe, ering is used to * track probe failures. Clear accumulated device error info. */ drivers/ata/libata-sata.c +1
-1@@ -1505,7 +1505,7 @@ int ata_eh_get_ncq_success_sense(struct ata_link *link) {struct ata_device *dev = link->device;struct ata_port *ap = dev->link->ap; -u8 *buf = ap->ncq_sense_buf; +u8 *buf = dev->cdl->ncq_sense_log_buf;struct ata_queued_cmd *qc;unsigned int err_mask, tag;u8 *sense, sk =0 , asc =0 , ascq =0 ;drivers/ata/libata-scsi.c +1
-1@@ -2259,7 +2259,7 @@ static inline u16 ata_xlat_cdl_limit(u8 *buf)static unsigned int ata_msense_control_spgt2 (struct ata_device *dev,u8 *buf,u8 spg) { -u8 *b, *cdl = dev->cdl, *desc; +u8 *b, *cdl = dev->cdl->desc_log_buf, *desc;u32 policy;int i;drivers/ata/libata.h +1
-0@@ -89,6 +89,7 @@ extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg);extern const char *sata_spd_string (unsigned int spd);extern unsigned int ata_read_log_page (struct ata_device *dev,u8 log,u8 page,void *buf,unsigned int sectors); +void ata_dev_cleanup_cdl_resources (struct ata_device *dev);#define to_ata_port (d) container_of(d, struct ata_port, tdev)include/linux/libata.h +17
-4@@ -700,6 +700,21 @@ struct ata_cpr_log { struct ata_cpr cpr[] __counted_by(nr_cpr); }; +struct ata_cdl { +/* +* Buffer to cache the CDL log page 18h (command duration descriptors) +* for SCSI-ATA translation. +*/ +u8 desc_log_buf[ATA_LOG_CDL_SIZE ]; + +/* +* Buffer to handle reading the sense data for successful NCQ Commands +* log page for commands using a CDL with one of the limits policy set +* to 0xD (successful completion with sense data available bit set). +*/ +u8 ncq_sense_log_buf[ATA_LOG_SENSE_NCQ_SIZE ]; +}; +struct ata_device {struct ata_link *link;unsigned int devno;/* 0 or 1 */ @@ -762,8 +777,8 @@ struct ata_device {/* Concurrent positioning ranges */ struct ata_cpr_log *cpr_log; -/* Command Duration Limits log support */ -u8 cdl[ATA_LOG_CDL_SIZE ]; +/* Command Duration Limits support */ +struct ata_cdl *cdl;/* error history */ int spdn_cnt;@@ -917,8 +932,6 @@ struct ata_port {#ifdef CONFIG_ATA_ACPI struct ata_acpi_gtm __acpi_init_gtm;/* use ata_acpi_init_gtm() */ #endif -/* owned by EH */ -u8 *ncq_sense_buf; };/* The following initializer overrides a method to NULL whether one of Signature · 1 line
2.46.0
Signature · 2 lines
Damien Le Moal Western Digital Research
Niklas Cassel
maintainerRe: [PATCH v3 7/7] ata: libata: Improve CDL resource management
Quoted 18 lines · Damien Le Moal
Quoted 16 lines · Niklas Cassel
Review static void ata_port_detach(struct ata_port *ap) · line 6025
Quoted 9 lines
(quoted diff) +5
-0@@ -6000,11 +6018,19 @@ static void ata_port_detach(struct ata_port *ap) ata_port_wait_eh(ap); mutex_lock(&ap->scsi_scan_mutex); + + /* Cleanup CDL device resources */ + ata_for_each_link(link, ap, HOST_FIRST) { + ata_for_each_dev(dev, link, ALL) + ata_dev_cleanup_cdl_resources(dev);Here you clean up resources.
Why?
Resources will get cleaned up whenata_port_free()is called, which will be called byata_devres_release()->ata_host_put()->ata_host_release()->ata_port_free(), when the device is removed.
Quoted 8 lines
That happens only if the host (=port) is removed, but not if only the device is being removed, e.g. because it was yanked out of its slot (hotplug) or the user removed it using sysfs. In such case, the port remains and is not deleted, so
ata_port_free()is not called. Ports exists for as long as the adapter exist.Quoted 1 line
I don't see any reason to free it here as well.
Hotplug of drives :)
?
This code is in ata_port_detach(), which is called only by ata_host_detach().
So the code you have added here is only when the port is detached, not when the device is detached / hotplug removed.
For hotplug it is these functions:ata_eh_detach_dev() and ata_scsi_handle_link_detach() that will be called.
We know that the struct ata_device is never freed until the port is freed, but if you want to be nice and free the device's resources on hotplug, then I think it is better to remove the freeing for the resources in ata_port_free(), and only perform it in ata_eh_dev_disable(), since ata_eh_dev_disable() is called both on hotplug removals (by ata_eh_detach_dev()) and on unload (by ata_eh_unload()).
Unload is performed by ata_port_detach(), so ata_eh_unload() -> ata_eh_dev_disable() will always be called, there is no need for an additional freeing in ata_port_detach() (or in ata_port_free()).
I suggest something like the following on top of your v4:
drivers/ata/libata-core.c +0 -18
@@ -5463,11 +5463,6 @@ void ata_port_free(struct ata_port *ap)if (!ap)return ; -ata_for_each_link (link, ap,HOST_FIRST ) { -ata_for_each_dev (dev, link,ALL ) -ata_dev_cleanup_cdl_resources (dev); - } -kfree (ap->pmp_link);kfree (ap->slave_link);ida_free (&ata_ida, ap->print_id);
@@ -6019,12 +6014,6 @@ static void ata_port_detach(struct ata_port *ap)mutex_lock (&ap->scsi_scan_mutex); -/* Cleanup CDL device resources */ -ata_for_each_link (link, ap,HOST_FIRST ) { -ata_for_each_dev (dev, link,ALL ) -ata_dev_cleanup_cdl_resources (dev); - } -spin_lock_irqsave (ap->lock, flags);/* Remove scsi devices */
@@ -6055,13 +6044,6 @@ static void ata_port_detach(struct ata_port *ap)cancel_delayed_work_sync (&ap->hotplug_task);cancel_delayed_work_sync (&ap->scsi_rescan_task); -/* clean up zpodd on port removal */ -ata_for_each_link (link, ap,HOST_FIRST ) { -ata_for_each_dev (dev, link,ALL ) { -if (zpodd_dev_enabled (dev)) -zpodd_exit (dev); - } - }if (ap->pmp_link) {int i;for (i =0 ; i <SATA_PMP_MAX_PORTS ; i++)
drivers/ata/libata-eh.c +9 -1
@@ -494,13 +494,21 @@ void ata_eh_release(struct ata_port *ap)mutex_unlock (&ap->host->eh_mutex); } +static void ata_eh_dev_free_resources (struct ata_device *dev) +{ +if (zpodd_dev_enabled (dev)) +zpodd_exit (dev); + +ata_dev_cleanup_cdl_resources (dev); +} +static void ata_eh_dev_disable (struct ata_device *dev) {ata_acpi_on_disable (dev);ata_down_xfermask_limit (dev,ATA_DNXFER_FORCE_PIO0 |ATA_DNXFER_QUIET ); dev->class++; -ata_dev_cleanup_cdl_resources (dev); +ata_eh_dev_free_resources (dev);/* From now till the next successful probe, ering is used to * track probe failures. Clear accumulated device error info.
drivers/ata/libata-scsi.c +0 -3
@@ -4612,9 +4612,6 @@ static void ata_scsi_handle_link_detach(struct ata_link *link) dev->flags &= ~ATA_DFLAG_DETACHED ;spin_unlock_irqrestore (ap->lock, flags); -if (zpodd_dev_enabled (dev)) -zpodd_exit (dev); -ata_scsi_remove_dev (dev); } }
So that we keep all the freeing of struct ata_device's resources in a single function.
Kind regards,
Niklas
Damien Le Moal
maintainerRe: [PATCH v3 7/7] ata: libata: Improve CDL resource management
Quoted 26 lines · Niklas Cassel
Quoted 25 lines · Damien Le Moal
Quoted 16 lines · Niklas Cassel
Review static void ata_port_detach(struct ata_port *ap) · line 6025
Quoted 9 lines
(quoted diff) +5
-0@@ -6000,11 +6018,19 @@ static void ata_port_detach(struct ata_port *ap) ata_port_wait_eh(ap); mutex_lock(&ap->scsi_scan_mutex); + + /* Cleanup CDL device resources */ + ata_for_each_link(link, ap, HOST_FIRST) { + ata_for_each_dev(dev, link, ALL) + ata_dev_cleanup_cdl_resources(dev);Here you clean up resources.
Why?
Resources will get cleaned up whenata_port_free()is called, which will be called byata_devres_release()->ata_host_put()->ata_host_release()->ata_port_free(), when the device is removed.That happens only if the host (=port) is removed, but not if only the device is being removed, e.g. because it was yanked out of its slot (hotplug) or the user removed it using sysfs. In such case, the port remains and is not deleted, so
ata_port_free()is not called. Ports exists for as long as the adapter exist.Quoted 1 line
I don't see any reason to free it here as well.
Quoted 8 lines
Quoted 1 line
Hotplug of drives :)
?
This code is in
ata_port_detach(), which is called only byata_host_detach().So the code you have added here is only when the port is detached, not when the device is detached / hotplug removed.
Indeed. I screwed-up with this one :)
Quoted 2 lines
For hotplug it is these functions:
ata_eh_detach_dev()andata_scsi_handle_link_detach()that will be called.
And that's the path I should have modified.
Quoted 98 lines
We know that the struct
ata_deviceis never freed until the port is freed, but if you want to be nice and free the device's resources on hotplug, then I think it is better to remove the freeing for the resources inata_port_free(), and only perform it inata_eh_dev_disable(), sinceata_eh_dev_disable()is called both on hotplug removals (byata_eh_detach_dev()) and on unload (byata_eh_unload()).Unload is performed by
ata_port_detach(), soata_eh_unload()->ata_eh_dev_disable()will always be called, there is no need for an additional freeing inata_port_detach()(or inata_port_free()).I suggest something like the following on top of your v4:
drivers/ata/libata-core.c +0
-18@@ -5463,11 +5463,6 @@ void ata_port_free(struct ata_port *ap)if (!ap)return ; -ata_for_each_link (link, ap,HOST_FIRST ) { -ata_for_each_dev (dev, link,ALL ) -ata_dev_cleanup_cdl_resources (dev); - } -kfree (ap->pmp_link);kfree (ap->slave_link);ida_free (&ata_ida, ap->print_id);@@ -6019,12 +6014,6 @@ static void ata_port_detach(struct ata_port *ap)mutex_lock (&ap->scsi_scan_mutex); -/* Cleanup CDL device resources */ -ata_for_each_link (link, ap,HOST_FIRST ) { -ata_for_each_dev (dev, link,ALL ) -ata_dev_cleanup_cdl_resources (dev); - } -spin_lock_irqsave (ap->lock, flags);/* Remove scsi devices */ @@ -6055,13 +6044,6 @@ static void ata_port_detach(struct ata_port *ap)cancel_delayed_work_sync (&ap->hotplug_task);cancel_delayed_work_sync (&ap->scsi_rescan_task); -/* clean up zpodd on port removal */ -ata_for_each_link (link, ap,HOST_FIRST ) { -ata_for_each_dev (dev, link,ALL ) { -if (zpodd_dev_enabled (dev)) -zpodd_exit (dev); - } - }if (ap->pmp_link) {int i;for (i =0 ; i <SATA_PMP_MAX_PORTS ; i++)drivers/ata/libata-eh.c +9
-1@@ -494,13 +494,21 @@ void ata_eh_release(struct ata_port *ap)mutex_unlock (&ap->host->eh_mutex); } +static void ata_eh_dev_free_resources (struct ata_device *dev) +{ +if (zpodd_dev_enabled (dev)) +zpodd_exit (dev); + +ata_dev_cleanup_cdl_resources (dev); +} +static void ata_eh_dev_disable (struct ata_device *dev) {ata_acpi_on_disable (dev);ata_down_xfermask_limit (dev,ATA_DNXFER_FORCE_PIO0 |ATA_DNXFER_QUIET ); dev->class++; -ata_dev_cleanup_cdl_resources (dev); +ata_eh_dev_free_resources (dev);/* From now till the next successful probe, ering is used to * track probe failures. Clear accumulated device error info. drivers/ata/libata-scsi.c +0
-3@@ -4612,9 +4612,6 @@ static void ata_scsi_handle_link_detach(struct ata_link *link) dev->flags &= ~ATA_DFLAG_DETACHED ;spin_unlock_irqrestore (ap->lock, flags); -if (zpodd_dev_enabled (dev)) -zpodd_exit (dev); -ata_scsi_remove_dev (dev); } }
Quoted 2 lines
So that we keep all the freeing of struct ata_device's resources in a single function.
Looks better. I will integrate that in patch 7. Thanks.
Quoted 4 lines
Kind regards,
Niklas
Signature · 2 lines
Damien Le Moal Western Digital Research