libata cleanups
Damien Le Moal
maintainercover libata cleanups
Here is a set of patches to cleanup a little libata code. Except for patch 1, there are no functional changes.
Damien Le Moal (5):
ata: libata-core: improve tag checks in ata_qc_issue()
ata: libata-sata: simplify ata_sas_queuecmd()
ata: libata-scsi: simplify ata_scsi_schedule_deferred_qc()
ata: libata-scsi: make ata_scsi_simulate() static
ata: libata-scsi: rename and improve ata_qc_done()
drivers/ata/libata-core.c | 9 +- drivers/ata/libata-sata.c | 14 ++- drivers/ata/libata-scsi.c | 174 +++++++++++++++++++------------------- include/linux/libata.h | 1 - 4 files changed, 98 insertions(+), 100 deletions(-)
Signature · 1 line
2.53.0
Damien Le Moal
maintainerPATCH 1/5 ata: libata-core: improve tag checks in ata_qc_issue()
Make sure to check that the tag of a queued command is valid when ata_qc_issue() is called, and fail the QC if the tag is not valid, or if there is an on-going non-NCQ command already on the link.
--- drivers/ata/libata-core.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/ata/libata-core.c +7 -2
@@ -5146,8 +5146,13 @@ void ata_qc_issue(struct ata_queued_cmd *qc)struct ata_link *link = qc->dev->link;u8 prot = qc->tf.protocol; -/* Make sure only one non-NCQ command is outstanding. */ -WARN_ON_ONCE (ata_tag_valid (link->active_tag)); +/* +* Make sure we have a valid tag and that only one non-NCQ command is +* outstanding. +*/ +if (WARN_ON_ONCE (!ata_tag_valid (qc->tag)) || +WARN_ON_ONCE (ata_tag_valid (link->active_tag))) + goto sys_err;if (ata_is_ncq (prot)) {WARN_ON_ONCE (link->sactive & (1 << qc->hw_tag));
Signature · 1 line
2.53.0
Re: [PATCH 1/5] ata: libata-core: improve tag checks in ata_qc_issue()
Quoted 9 lines · Damien Le Moal
Make sure to check that the tag of a queued command is valid when
ata_qc_issue()is called, and fail the QC if the tag is not valid, or if there is an on-going non-NCQ command already on the link.Signed-off-by Damien Le Moal <[email protected]>--- drivers/ata/libata-core.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 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
Damien Le Moal
maintainerPATCH 2/5 ata: libata-sata: simplify ata_sas_queuecmd()
In reply to Damien Le MoalChange ata_sas_queuecmd() to return early the result of __ata_scsi_queuecmd() and remove the rc local variable. This simplifies the code without any functional change.
--- drivers/ata/libata-sata.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
drivers/ata/libata-sata.c +6 -8
@@ -1378,15 +1378,13 @@ EXPORT_SYMBOL_GPL(ata_sas_sdev_configure);int ata_sas_queuecmd (struct scsi_cmnd *cmd,struct ata_port *ap) { -int rc =0 ; -if (likely (ata_dev_enabled (ap->link.device))) - rc =__ata_scsi_queuecmd (cmd, ap->link.device); -else { - cmd->result = (DID_BAD_TARGET <<16 ); -scsi_done (cmd); - } -return rc; +return __ata_scsi_queuecmd (cmd, ap->link.device); + + cmd->result = (DID_BAD_TARGET <<16 ); +scsi_done (cmd); + +return 0 ; }EXPORT_SYMBOL_GPL (ata_sas_queuecmd);
Signature · 1 line
2.53.0
Re: [PATCH 2/5] ata: libata-sata: simplify ata_sas_queuecmd()
Quoted 9 lines · Damien Le Moal
Change
ata_sas_queuecmd()to return early the result of__ata_scsi_queuecmd()and remove the rc local variable. This simplifies the code without any functional change.Signed-off-by Damien Le Moal <[email protected]>--- drivers/ata/libata-sata.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 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
Damien Le Moal
maintainerPATCH 3/5 ata: libata-scsi: simplify ata_scsi_schedule_deferred_qc()
In reply to Damien Le MoalUse ata_qc_done() instead of calling ata_qc_free() and scsi_done() directly.
--- drivers/ata/libata-scsi.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
drivers/ata/libata-scsi.c +2 -5
@@ -1685,7 +1685,6 @@ void ata_scsi_deferred_qc_work(struct work_struct *work)void ata_scsi_requeue_deferred_qc (struct ata_port *ap) {struct ata_queued_cmd *qc = ap->deferred_qc; -struct scsi_cmnd *scmd;lockdep_assert_held (ap->lock);
@@ -1697,11 +1696,9 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)if (!qc)return ; - scmd = qc->scsicmd; ap->deferred_qc =NULL ; -ata_qc_free (qc); - scmd->result = (DID_SOFT_ERROR <<16 ); -scsi_done (scmd); + qc->scsicmd->result = (DID_SOFT_ERROR <<16 ); +ata_qc_done (qc); }static void ata_scsi_schedule_deferred_qc (struct ata_port *ap)
Signature · 1 line
2.53.0
Re: [PATCH 3/5] ata: libata-scsi: simplify ata_scsi_schedule_deferred_qc()
Quoted 13 lines · Damien Le Moal
Use
ata_qc_done()instead of callingata_qc_free()andscsi_done()directly.Signed-off-by Damien Le Moal <[email protected]>--- drivers/ata/libata-scsi.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)drivers/ata/libata-scsi.c +0
-0
drivers/ata/libata-scsi.c void ata_scsi_deferred_qc_work(struct work_struct *work) · line 1685
Quoted 1 line
drivers/ata/libata-scsi.c +0
-0@@ -1685,7 +1685,6 @@ void ata_scsi_deferred_qc_work(struct work_struct *work)void ata_scsi_requeue_deferred_qc (struct ata_port *ap)
You're simplifying ata_scsi_requeue_deferred_qc() but the subject hasata_scsi_schedule_deferred_qc()...
Quoted 10 lines
drivers/ata/libata-scsi.c +0
-1{struct ata_queued_cmd *qc = ap->deferred_qc; -struct scsi_cmnd *scmd;lockdep_assert_held (ap->lock);@@ -1697,11 +1696,9 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)if (!qc)return ;
drivers/ata/libata-scsi.c void ata_scsi_requeue_deferred_qc(struct ata_port *ap) · line 1704
Quoted 10 lines
drivers/ata/libata-scsi.c +2
-4- scmd = qc->scsicmd; ap->deferred_qc =NULL ; -ata_qc_free (qc); - scmd->result = (DID_SOFT_ERROR <<16 ); -scsi_done (scmd); + qc->scsicmd->result = (DID_SOFT_ERROR <<16 ); +ata_qc_done (qc); }static void ata_scsi_schedule_deferred_qc (struct ata_port *ap)
MBR, Sergey
Re: [PATCH 3/5] ata: libata-scsi: simplify ata_scsi_schedule_deferred_qc()
In reply to Damien Le MoalQuoted 8 lines · Damien Le Moal
Use
ata_qc_done()instead of callingata_qc_free()andscsi_done()directly.Signed-off-by Damien Le Moal <[email protected]>--- drivers/ata/libata-scsi.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
As noted by Sergey, the description refers to the wrong function.
Other than that:
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
Damien Le Moal
maintainerRe: [PATCH 3/5] ata: libata-scsi: simplify ata_scsi_schedule_deferred_qc()
Quoted 11 lines · Hannes Reinecke
Quoted 8 lines · Damien Le Moal
Use
ata_qc_done()instead of callingata_qc_free()andscsi_done()directly.Signed-off-by Damien Le Moal <[email protected]>--- drivers/ata/libata-scsi.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)As noted by Sergey, the description refers to the wrong function.
Oops. Corrected.
Quoted 3 lines
Other than that:
Reviewed-by Hannes Reinecke <[email protected]>
Thanks.
Quoted 4 lines
Cheers,
Hannes
Signature · 2 lines
Damien Le Moal Western Digital Research
Damien Le Moal
maintainerPATCH 4/5 ata: libata-scsi: make ata_scsi_simulate() static
In reply to Damien Le Moalata_scsi_simulate() is called only from libata-scsi.c. Move this function definition as a static function before its call in __ata_scsi_queuecmd() and remove its declaration from include/linux/libata.h.
No functional changes.
--- drivers/ata/libata-scsi.c | 147 +++++++++++++++++++------------------- include/linux/libata.h | 1 - 2 files changed, 73 insertions(+), 75 deletions(-)
drivers/ata/libata-scsi.c +73 -74
@@ -4420,6 +4420,79 @@ static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd)return NULL ; } +/** +* ata_scsi_simulate - simulate SCSI command on ATA device +* @dev: the target device +* @cmd: SCSI command being sent to device. +* +* Interprets and directly executes a select list of SCSI commands +* that can be handled internally. +* +* LOCKING: +* spin_lock_irqsave(host lock) +*/ +static void ata_scsi_simulate (struct ata_device *dev,struct scsi_cmnd *cmd) +{ +const u8 *scsicmd = cmd->cmnd; +u8 tmp8; + +switch (scsicmd[0 ]) { +case INQUIRY : +ata_scsi_rbuf_fill (dev, cmd, ata_scsiop_inquiry); +break ; + +case MODE_SENSE : +case MODE_SENSE_10 : +ata_scsi_rbuf_fill (dev, cmd, ata_scsiop_mode_sense); +break ; + +case READ_CAPACITY : +case SERVICE_ACTION_IN_16 : +ata_scsi_rbuf_fill (dev, cmd, ata_scsiop_read_cap); +break ; + +case REPORT_LUNS : +ata_scsi_rbuf_fill (dev, cmd, ata_scsiop_report_luns); +break ; + +case REQUEST_SENSE : +ata_scsi_set_sense (dev, cmd,0 ,0 ,0 ); +break ; + +/* if we reach this, then writeback caching is disabled, +* turning this into a no-op. +*/ +case SYNCHRONIZE_CACHE : +case SYNCHRONIZE_CACHE_16 : + fallthrough; + +/* no-op's, complete with success */ +case REZERO_UNIT : +case SEEK_6 : +case SEEK_10 : +case TEST_UNIT_READY : +break ; + +case SEND_DIAGNOSTIC : + tmp8 = scsicmd[1 ] & ~(1 <<3 ); +if (tmp8 !=0x4 || scsicmd[3 ] || scsicmd[4 ]) +ata_scsi_set_invalid_field (dev, cmd,1 ,0xff ); +break ; + +case MAINTENANCE_IN : +ata_scsi_rbuf_fill (dev, cmd, ata_scsiop_maint_in); +break ; + +/* all other commands */ +default : +ata_scsi_set_sense (dev, cmd,ILLEGAL_REQUEST ,0x20 ,0x0 ); +/* "Invalid command operation code" */ +break ; + } + +scsi_done (cmd); +} +enum scsi_qc_status __ata_scsi_queuecmd (struct scsi_cmnd *scmd,struct ata_device *dev) {
@@ -4522,80 +4595,6 @@ enum scsi_qc_status ata_scsi_queuecmd(struct Scsi_Host *shost, }EXPORT_SYMBOL_GPL (ata_scsi_queuecmd); -/** -* ata_scsi_simulate - simulate SCSI command on ATA device -* @dev: the target device -* @cmd: SCSI command being sent to device. -* -* Interprets and directly executes a select list of SCSI commands -* that can be handled internally. -* -* LOCKING: -* spin_lock_irqsave(host lock) -*/ - -void ata_scsi_simulate (struct ata_device *dev,struct scsi_cmnd *cmd) -{ -const u8 *scsicmd = cmd->cmnd; -u8 tmp8; - -switch (scsicmd[0 ]) { -case INQUIRY : -ata_scsi_rbuf_fill (dev, cmd, ata_scsiop_inquiry); -break ; - -case MODE_SENSE : -case MODE_SENSE_10 : -ata_scsi_rbuf_fill (dev, cmd, ata_scsiop_mode_sense); -break ; - -case READ_CAPACITY : -case SERVICE_ACTION_IN_16 : -ata_scsi_rbuf_fill (dev, cmd, ata_scsiop_read_cap); -break ; - -case REPORT_LUNS : -ata_scsi_rbuf_fill (dev, cmd, ata_scsiop_report_luns); -break ; - -case REQUEST_SENSE : -ata_scsi_set_sense (dev, cmd,0 ,0 ,0 ); -break ; - -/* if we reach this, then writeback caching is disabled, -* turning this into a no-op. -*/ -case SYNCHRONIZE_CACHE : -case SYNCHRONIZE_CACHE_16 : - fallthrough; - -/* no-op's, complete with success */ -case REZERO_UNIT : -case SEEK_6 : -case SEEK_10 : -case TEST_UNIT_READY : -break ; - -case SEND_DIAGNOSTIC : - tmp8 = scsicmd[1 ] & ~(1 <<3 ); -if (tmp8 !=0x4 || scsicmd[3 ] || scsicmd[4 ]) -ata_scsi_set_invalid_field (dev, cmd,1 ,0xff ); -break ; - -case MAINTENANCE_IN : -ata_scsi_rbuf_fill (dev, cmd, ata_scsiop_maint_in); -break ; - -/* all other commands */ -default : -ata_scsi_set_sense (dev, cmd,ILLEGAL_REQUEST ,0x20 ,0x0 ); -/* "Invalid command operation code" */ -break ; - } - -scsi_done (cmd); -} -int ata_scsi_add_hosts (struct ata_host *host,const struct scsi_host_template *sht) {int i, rc;
include/linux/libata.h +0 -1
@@ -1205,7 +1205,6 @@ extern unsigned int ata_do_dev_read_id(struct ata_device *dev, struct ata_taskfile *tf, __le16 *id);extern void ata_qc_complete (struct ata_queued_cmd *qc);extern u64 ata_qc_get_active (struct ata_port *ap); -extern void ata_scsi_simulate (struct ata_device *dev,struct scsi_cmnd *cmd);extern int ata_std_bios_param (struct scsi_device *sdev,struct gendisk *unused,sector_t capacity,int geom[]);
Signature · 1 line
2.53.0
Re: [PATCH 4/5] ata: libata-scsi: make ata_scsi_simulate() static
Quoted 5 lines · Damien Le Moal
ata_scsi_simulate()is called only fromlibata-scsi.c. Move this function definition as a static function before its call in__ata_scsi_queuecmd()and remove its declaration frominclude/linux/libata.h.
Quoted 8 lines
No functional changes.
Signed-off-by Damien Le Moal <[email protected]>--- drivers/ata/libata-scsi.c | 147 +++++++++++++++++++------------------- include/linux/libata.h | 1 - 2 files changed, 73 insertions(+), 75 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
Damien Le Moal
maintainerPATCH 5/5 ata: libata-scsi: rename and improve ata_qc_done()
In reply to Damien Le MoalRename ata_qc_done() to ata_scsi_qc_done() and allow to pass a scsi command result value to set for the completed command to simplify the caller sites.
No functional changes.
--- drivers/ata/libata-scsi.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
drivers/ata/libata-scsi.c +12 -12
@@ -1649,12 +1649,16 @@ static unsigned int ata_scsi_rw_xlat(struct ata_queued_cmd *qc)return 1 ; } -static void ata_qc_done (struct ata_queued_cmd *qc) +static void ata_scsi_qc_done (struct ata_queued_cmd *qc,bool set_result, +u32 scmd_result) {struct scsi_cmnd *cmd = qc->scsicmd;void (*done)(struct scsi_cmnd *) = qc->scsidone;ata_qc_free (qc); + +if (set_result) + cmd->result = scmd_result;done (cmd); }
@@ -1693,12 +1697,10 @@ void ata_scsi_requeue_deferred_qc(struct ata_port *ap)* do not try to be smart about what to do with this deferred command * and simply retry it by completing it with DID_SOFT_ERROR. */ -if (!qc) -return ; - - ap->deferred_qc =NULL ; - qc->scsicmd->result = (DID_SOFT_ERROR <<16 ); -ata_qc_done (qc); +if (qc) { + ap->deferred_qc =NULL ; +ata_scsi_qc_done (qc, true,DID_SOFT_ERROR <<16 ); + } }static void ata_scsi_schedule_deferred_qc (struct ata_port *ap)
@@ -1754,7 +1756,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)ata_scsi_set_sense_information (qc); } -ata_qc_done (qc); +ata_scsi_qc_done (qc, false,0 );ata_scsi_schedule_deferred_qc (ap); }
@@ -2913,17 +2915,15 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)if (qc->cdb[0 ] ==ALLOW_MEDIUM_REMOVAL && qc->dev->sdev) qc->dev->sdev->locked =0 ; - qc->scsicmd->result =SAM_STAT_CHECK_CONDITION ; -ata_qc_done (qc); +ata_scsi_qc_done (qc, true,SAM_STAT_CHECK_CONDITION );return ; }/* successful completion path */ if (cmd->cmnd[0 ] ==INQUIRY && (cmd->cmnd[1 ] &0x03 ) ==0 )atapi_fixup_inquiry (cmd); - cmd->result =SAM_STAT_GOOD ; -ata_qc_done (qc); +ata_scsi_qc_done (qc, true,SAM_STAT_GOOD ); }/** * atapi_xlat - Initialize PACKET taskfile
Signature · 1 line
2.53.0
Re: [PATCH 5/5] ata: libata-scsi: rename and improve ata_qc_done()
Quoted 11 lines · Damien Le Moal
Rename
ata_qc_done()toata_scsi_qc_done()and allow to pass a scsi command result value to set for the completed command to simplify the caller sites.No functional changes.
Signed-off-by Damien Le Moal <[email protected]>--- drivers/ata/libata-scsi.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 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
Quoted 10 lines · Damien Le Moal
Here is a set of patches to cleanup a little libata code. Except for patch 1, there are no functional changes.
Damien Le Moal (5):
ata: libata-core: improve tag checks inata_qc_issue()
ata: libata-sata: simplifyata_sas_queuecmd()
ata: libata-scsi: simplifyata_scsi_schedule_deferred_qc()
ata: libata-scsi: makeata_scsi_simulate()static
ata: libata-scsi: rename and improveata_qc_done()
Quoted 5 lines
drivers/ata/libata-core.c | 9 +- drivers/ata/libata-sata.c | 14 ++- drivers/ata/libata-scsi.c | 174 +++++++++++++++++++------------------- include/linux/libata.h | 1 - 4 files changed, 98 insertions(+), 100 deletions(-)
Applied to for-7.1.
Signature · 2 lines
Damien Le Moal Western Digital Research