Code cleanup and memory usage reduction
Damien Le Moal
maintainerv5 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 improves device flag manipulation inata_scsi_handle_link_detach().
Patch 3 introduces a small simplification/improvement of __ata_qc_complete().
Patches 4 and 5 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 6 renames some functions to make it clearer what the functions do.
Finally, patch 7 to 9 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 V4:
- Added patch 2 and 8
- Modified patch 9 to use the ata_dev_free_resources() function introduced in patch 8.
Changes from V3:
- Small change to struct ata_cdl kzalloc() call as suggested by Niklas
- Removed bogus call to ata_dev_cleanup_cdl_resources() in patch 7
- Added review tags
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 (9):
ata: libata: Cleanup libata-transport
ata: libata-scsi: Improve ata_scsi_handle_link_detach()
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: Introduce ata_dev_free_resources
ata: libata: Improve CDL resource management
drivers/ata/libata-core.c | 278 ++++++++++-------------------- drivers/ata/libata-eh.c | 13 +- drivers/ata/libata-pmp.c | 3 +- drivers/ata/libata-sata.c | 127 +++++++++++++- drivers/ata/libata-scsi.c | 11 +- drivers/ata/libata-transport.c | 299 ++++++++++++++++----------------- drivers/ata/libata-zpodd.c | 2 +- drivers/ata/libata.h | 24 ++- include/linux/libata.h | 39 +++-- 9 files changed, 419 insertions(+), 377 deletions(-)
Signature · 1 line
2.46.0
Damien Le Moal
maintainerPATCH v5 1/9 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
Damien Le Moal
maintainerPATCH v5 2/9 ata: libata-scsi: Improve ata_scsi_handle_link_detach()
In reply to Damien Le MoalA struct ata_device flags are always set iand cleared with the device port locked. Testing for a flag should thus also be done while holding the device port lock. In accordance to this, modify ata_scsi_handle_link_detach() to both test and clear the ATA_DFLAG_DETACHED flag while holding the device port lock.
--- drivers/ata/libata-scsi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/ata/libata-scsi.c +4 -2
@@ -4616,10 +4616,12 @@ static void ata_scsi_handle_link_detach(struct ata_link *link)ata_for_each_dev (dev, link,ALL ) {unsigned long flags; -if (!(dev->flags &ATA_DFLAG_DETACHED )) +spin_lock_irqsave (ap->lock, flags); +if (!(dev->flags &ATA_DFLAG_DETACHED )) { +spin_unlock_irqrestore (ap->lock, flags);continue ; + } -spin_lock_irqsave (ap->lock, flags); dev->flags &= ~ATA_DFLAG_DETACHED ;spin_unlock_irqrestore (ap->lock, flags);
Signature · 1 line
2.46.0
Re: [PATCH v5 2/9] ata: libata-scsi: Improve ata_scsi_handle_link_detach()
Quoted 1 line · Damien Le Moal
A struct
ata_deviceflags are always set iand cleared with the device
s/iand/and/
I can see many cases function that currently set or clear dev->flags withouth holding ap->lock, done by functions that run in EH context, which will thus not hold ap->lock.
Perhaps rephrase this to:
struct ata_device flags should always be set and cleared with the device
Quoted 32 lines
port locked. Testing for a flag should thus also be done while holding the device port lock. In accordance to this, modify
ata_scsi_handle_link_detach()to both test and clear theATA_DFLAG_DETACHEDflag while holding the device port lock.Signed-off-by Damien Le Moal <[email protected]>--- drivers/ata/libata-scsi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/ata/libata-scsi.c +4
-2@@ -4616,10 +4616,12 @@ static void ata_scsi_handle_link_detach(struct ata_link *link)ata_for_each_dev (dev, link,ALL ) {unsigned long flags; -if (!(dev->flags &ATA_DFLAG_DETACHED )) +spin_lock_irqsave (ap->lock, flags); +if (!(dev->flags &ATA_DFLAG_DETACHED )) { +spin_unlock_irqrestore (ap->lock, flags);continue ; + } -spin_lock_irqsave (ap->lock, flags); dev->flags &= ~ATA_DFLAG_DETACHED ;spin_unlock_irqrestore (ap->lock, flags);Signature · 1 line
2.46.0
Damien Le Moal
maintainerPATCH v5 3/9 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 v5 4/9 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 v5 5/9 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 v5 6/9 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 v5 7/9 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
Damien Le Moal
maintainerPATCH v5 8/9 ata: libata: Introduce ata_dev_free_resources
In reply to Damien Le MoalIntroduce the function ata_dev_free_resources() to free the resources allocated to support a device features. For now, this function is reduced to calling zpodd_exit() for devices that have this feature enabled.
ata_dev_free_resources() is called from ata_eh_dev_disable() as this function is always called for all devices attached to a port that is being detached and for devices that are being disabled due to being removed (detached) from the system or due to errors.
With this change, the call to zpodd_exit() done in ata_port_detach() is removed as that function starts by removing all devices attached to the port using libata EH, thus resulting in ata_eh_dev_disable() being called and the zpodd_exit() function being executed.
--- drivers/ata/libata-core.c | 27 +++++++++++++++++++-------- drivers/ata/libata-eh.c | 5 ++++- drivers/ata/libata-scsi.c | 3 --- drivers/ata/libata.h | 1 + 4 files changed, 24 insertions(+), 12 deletions(-)
drivers/ata/libata-core.c +19 -8
@@ -5976,6 +5976,21 @@ int ata_host_activate(struct ata_host *host, int irq, }EXPORT_SYMBOL_GPL (ata_host_activate); +/** +* ata_dev_free_resources - Free a device resources +* @dev: Target ATA device +* +* Free resources allocated to support a device features. +* +* LOCKING: +* Kernel thread context (may sleep). +*/ +void ata_dev_free_resources (struct ata_device *dev) +{ +if (zpodd_dev_enabled (dev)) +zpodd_exit (dev); +} +/** * ata_port_detach - Detach ATA port in preparation of device removal * @ap: ATA port to be detached
@@ -6030,19 +6045,15 @@ 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); - } - } +/* Delete port multiplier link transport devices */ if (ap->pmp_link) {int i; +for (i =0 ; i <SATA_PMP_MAX_PORTS ; i++)ata_tlink_delete (&ap->pmp_link[i]); } -/* remove the associated SCSI host */ + +/* Remove the associated SCSI host */ scsi_remove_host (ap->scsi_host);ata_tport_delete (ap); }
drivers/ata/libata-eh.c +4 -1
@@ -500,10 +500,13 @@ static void ata_eh_dev_disable(struct ata_device *dev)ata_down_xfermask_limit (dev,ATA_DNXFER_FORCE_PIO0 |ATA_DNXFER_QUIET ); dev->class++; -/* From now till the next successful probe, ering is used to +/* +* From now till the next successful probe, ering is used to * track probe failures. Clear accumulated device error info. */ ata_ering_clear (&dev->ering); + +ata_dev_free_resources (dev); }static void ata_eh_unload (struct ata_port *ap)
drivers/ata/libata-scsi.c +0 -3
@@ -4625,9 +4625,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); } }
drivers/ata/libata.h +1 -0
@@ -71,6 +71,7 @@ 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); +void ata_dev_free_resources (struct ata_device *dev);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);
Signature · 1 line
2.46.0
Re: [PATCH v5 8/9] ata: libata: Introduce ata_dev_free_resources
Quoted 10 lines · Damien Le Moal
Introduce the function
ata_dev_free_resources()to free the resources allocated to support a device features. For now, this function is reduced to callingzpodd_exit()for devices that have this feature enabled.
ata_dev_free_resources()is called fromata_eh_dev_disable()as this function is always called for all devices attached to a port that is being detached and for devices that are being disabled due to being removed (detached) from the system or due to errors.
Quoted 7 lines
With this change, the call to
zpodd_exit()done inata_port_detach()is removed as that function starts by removing all devices attached to the port using libata EH, thus resulting inata_eh_dev_disable()being called and thezpodd_exit()function being executed.Signed-off-by Damien Le Moal <[email protected]>---
Damien Le Moal
maintainerPATCH v5 9/9 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 freeing 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_dev_free_resources() to free the ata_cdl structure when a device is being disabled by EH.
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 | 60 +++++++++++++++++++++++---------------- drivers/ata/libata-sata.c | 2 +- drivers/ata/libata-scsi.c | 2 +- drivers/ata/libata.h | 1 + include/linux/libata.h | 21 +++++++++++--- 5 files changed, 56 insertions(+), 30 deletions(-)
drivers/ata/libata-core.c +36 -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 (*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)
@@ -5451,7 +5462,6 @@ void ata_port_free(struct ata_port *ap)kfree (ap->pmp_link);kfree (ap->slave_link); -kfree (ap->ncq_sense_buf);ida_free (&ata_ida, ap->print_id);kfree (ap); }
@@ -5989,6 +5999,8 @@ void ata_dev_free_resources(struct ata_device *dev) {if (zpodd_dev_enabled (dev))zpodd_exit (dev); + +ata_dev_cleanup_cdl_resources (dev); }/**
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
@@ -90,6 +90,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
Re: [PATCH v5 9/9] ata: libata: Improve CDL resource management
Quoted 51 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 freeing 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_dev_free_resources()to free theata_cdlstructure when a device is being disabled by EH.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]>--- drivers/ata/libata-core.c | 60 +++++++++++++++++++++++---------------- drivers/ata/libata-sata.c | 2 +- drivers/ata/libata-scsi.c | 2 +- drivers/ata/libata.h | 1 + include/linux/libata.h | 21 +++++++++++--- 5 files changed, 56 insertions(+), 30 deletions(-)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 (*cdl),GFP_KERNEL );
drivers/ata/libata-core.c static void ata_dev_config_trusted(struct ata_device *dev) · line 2483
Quoted 10 lines
drivers/ata/libata-core.c +10
-0+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 ;
Usually when a function return error, you expect it to have freed the resources that it might have allocated, so perhaps free and set dev->cdl to NULL here.
Quoted 51 lines
drivers/ata/libata-core.c +16
-20+ } + +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; }
drivers/ata/libata-core.c static void ata_dev_config_cdl(struct ata_device *dev) · line 2608
Quoted 10 lines
drivers/ata/libata-core.c +1
-3-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);
Considering that you now do ata_dev_init_cdl_resources() at the end, if you implement my suggestion above, we can remove the call to ata_dev_cleanup_cdl_resources() here, since we will only jump here if the ata_dev_init_cdl_resources() call failed.
Either way:
Quoted 3 lines
drivers/ata/libata-core.c +0
-0}static int ata_dev_config_lba (struct ata_device *dev)
Damien Le Moal
maintainerRe: [PATCH v5 9/9] ata: libata: Improve CDL resource management
Quoted 13 lines · Niklas Cassel
Quoted 13 lines
drivers/ata/libata-core.c +13
-0+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 (*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,
Quoted 8 lines
drivers/ata/libata-core.c static void ata_dev_config_trusted(struct ata_device *dev) · line 2483
Quoted 4 lines
drivers/ata/libata-core.c +4
-0+ ATA_LOG_CDL_SIZE / ATA_SECT_SIZE); +if (err_mask) { +ata_dev_warn (dev,"Read Command Duration Limits log failed\n" ); +return -EIO ;Usually when a function return error, you expect it to have freed the resources that it might have allocated, so perhaps free and set
dev->cdlto NULL here.
Indeed. I added a call to ata_dev_cleanup_cdl_resources().
Quoted 35 lines
Quoted 35 lines
drivers/ata/libata-core.c +4
-20@@ -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 ;
Quoted 10 lines
drivers/ata/libata-core.c static void ata_dev_config_cdl(struct ata_device *dev) · line 2608
Quoted 5 lines
drivers/ata/libata-core.c +1
-2not_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);Considering that you now do
ata_dev_init_cdl_resources()at the end, if you implement my suggestion above, we can remove the call toata_dev_cleanup_cdl_resources()here, since we will only jump here if theata_dev_init_cdl_resources()call failed.
Nope. I think we need to keep that call because this function is going to be called again for device revalidation, with the CDL resource already allocated. So if we hit an error before reaching ata_dev_init_cdl_resources(), we should free the resources. So I prefer to keep it.
Quoted 2 lines
Either way:
Reviewed-by Niklas Cassel <[email protected]>
Thanks.
Signature · 2 lines
Damien Le Moal Western Digital Research