| Message ID | 20230720004257.307031-1-nks@flawful.org |
|---|---|
| Headers | show |
| Series | libata: remove references to 'old' error handler | expand |
On 2023/7/20 8:42, Niklas Cassel wrote: > From: Hannes Reinecke <hare@suse.de> > > Just used in one place. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > --- > drivers/ata/libata-core.c | 11 ++--------- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index 1f0306522649..c5e93e1a560d 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -5884,14 +5884,6 @@ void __ata_port_probe(struct ata_port *ap) > spin_unlock_irqrestore(ap->lock, flags); > } > > -int ata_port_probe(struct ata_port *ap) > -{ > - __ata_port_probe(ap); > - ata_port_wait_eh(ap); > - return 0; > -} > - > - > static void async_port_probe(void *data, async_cookie_t cookie) > { > struct ata_port *ap = data; > @@ -5906,7 +5898,8 @@ static void async_port_probe(void *data, async_cookie_t cookie) > if (!(ap->host->flags & ATA_HOST_PARALLEL_SCAN) && ap->port_no != 0) > async_synchronize_cookie(cookie); > > - (void)ata_port_probe(ap); > + __ata_port_probe(ap); > + ata_port_wait_eh(ap); > > /* in order to keep device order, we need to synchronize at this point */ > async_synchronize_cookie(cookie); > Reviewed-by: Jason Yan <yanaijie@huawei.com> Thanks, Jason
On 2023/7/20 8:42, Niklas Cassel wrote: > From: Niklas Cassel <niklas.cassel@wdc.com> > > The TODO claims that the pdc_20621_ops should set the .inherits > function pointer to &ata_base_port_ops after it has been converted > to use the new EH. > > However, the driver was converted to use the new EH a long time ago, > in commit 67651ee5710c ("[libata] sata_sx4: convert to new exception > handling methods"), which also did set .inherits function pointer to > &ata_sff_port_ops (and ata_sff_port_ops itself has .inherits set to > &ata_base_port_ops). > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > --- > drivers/ata/sata_sx4.c | 1 - > 1 file changed, 1 deletion(-) Reviewed-by: Jason Yan <yanaijie@huawei.com> Thanks, Jason
On 7/20/23 02:42, Niklas Cassel wrote: > From: Niklas Cassel <niklas.cassel@wdc.com> > > The TODO claims that the pdc_20621_ops should set the .inherits > function pointer to &ata_base_port_ops after it has been converted > to use the new EH. > > However, the driver was converted to use the new EH a long time ago, > in commit 67651ee5710c ("[libata] sata_sx4: convert to new exception > handling methods"), which also did set .inherits function pointer to > &ata_sff_port_ops (and ata_sff_port_ops itself has .inherits set to > &ata_base_port_ops). > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> > --- > drivers/ata/sata_sx4.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/ata/sata_sx4.c b/drivers/ata/sata_sx4.c > index ccc016072637..b51d7a9d0d90 100644 > --- a/drivers/ata/sata_sx4.c > +++ b/drivers/ata/sata_sx4.c > @@ -232,7 +232,6 @@ static const struct scsi_host_template pdc_sata_sht = { > .dma_boundary = ATA_DMA_BOUNDARY, > }; > > -/* TODO: inherit from base port_ops after converting to new EH */ > static struct ata_port_operations pdc_20621_ops = { > .inherits = &ata_sff_port_ops, > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On 20/07/2023 01:42, Niklas Cassel wrote: > From: Hannes Reinecke <hare@suse.de> > > With commit 65a15d6560df ("scsi: ipr: Remove SATA support") all > libata drivers now have the error_handler() callback provided, > so we can stop checking for non-existing error_handler callback. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > [niklas: fixed review comments, rebased, solved conflicts during rebase, > fixed bug that unconditionally dumped all QCs, removed the now unused > function ata_dump_status(), removed the now unreachable failure paths in > atapi_qc_complete(), removed the non-EH function to request ATAPI sense] > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> ata_qc_from_tag() still has a ap->ops->error_handler check, right? > --- > drivers/ata/libata-core.c | 209 +++++++++++++++----------------------- > drivers/ata/libata-eh.c | 150 ++++++++++++--------------- > drivers/ata/libata-sata.c | 7 +- > drivers/ata/libata-scsi.c | 142 ++------------------------ > drivers/ata/libata-sff.c | 30 ++---- > 5 files changed, 166 insertions(+), 372 deletions(-) > ... > > /* > - * For new EH, all qcs are finished in one of three ways - > + * For EH, all qcs are finished in one of three ways - > * normal completion, error completion, and SCSI timeout. > * Both completions can race against SCSI timeout. When normal > * completion wins, the qc never reaches EH. When error > @@ -659,94 +656,89 @@ EXPORT_SYMBOL(ata_scsi_cmd_error_handler); > void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap) > { > unsigned long flags; > + struct ata_link *link; > > /* invoke error handler */ Is this comment only really relevant when we may not previously invoked the error handler? > - if (ap->ops->error_handler) { > - struct ata_link *link; > > - /* acquire EH ownership */ > - ata_eh_acquire(ap); > + /* acquire EH ownership */ > + ata_eh_acquire(ap); > repeat: > - /* kill fast drain timer */ > - del_timer_sync(&ap->fastdrain_timer); > + /* kill fast drain timer */ > + del_timer_sync(&ap->fastdrain_timer); > > - /* process port resume request */ > - ata_eh_handle_port_resume(ap); > + /* process port resume request */ > + ata_eh_handle_port_resume(ap); > > - /* fetch & clear EH info */ > - spin_lock_irqsave(ap->lock, flags); > + /* fetch & clear EH info */ > + spin_lock_irqsave(ap->lock, flags); ... > * ata_to_sense_error - convert ATA error to SCSI error > * @id: ATA device number > @@ -904,7 +863,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > struct ata_taskfile *tf = &qc->result_tf; > unsigned char *sb = cmd->sense_buffer; > unsigned char *desc = sb + 8; > - int verbose = qc->ap->ops->error_handler == NULL; > u8 sense_key, asc, ascq; > > memset(sb, 0, SCSI_SENSE_BUFFERSIZE); > @@ -916,7 +874,7 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > if (qc->err_mask || > tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { > ata_to_sense_error(qc->ap->print_id, tf->status, tf->error, > - &sense_key, &asc, &ascq, verbose); > + &sense_key, &asc, &ascq, false); > ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq); > } else { > /* > @@ -999,7 +957,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) > struct scsi_cmnd *cmd = qc->scsicmd; > struct ata_taskfile *tf = &qc->result_tf; > unsigned char *sb = cmd->sense_buffer; > - int verbose = qc->ap->ops->error_handler == NULL; > u64 block; > u8 sense_key, asc, ascq; > > @@ -1017,7 +974,7 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) > if (qc->err_mask || > tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { > ata_to_sense_error(qc->ap->print_id, tf->status, tf->error, > - &sense_key, &asc, &ascq, verbose); > + &sense_key, &asc, &ascq, false); Please check this - AFAICS, we only ever pass false for @verbose arg now (so it would not be needed, and ata_to_sense_error() may be simplified) > ata_scsi_set_sense(dev, cmd, sense_key, asc, ascq); > } else { > /* Could not decode error */ > @@ -1179,9 +1136,6 @@ void ata_scsi_slave_destroy(struct scsi_device *sdev) > unsigned long flags; > struct ata_device *dev; > > - if (!ap->ops->error_handler)
On Thu, Jul 20, 2023 at 09:47:08AM +0100, John Garry wrote: > On 20/07/2023 01:42, Niklas Cassel wrote: > > From: Hannes Reinecke <hare@suse.de> > > > > With commit 65a15d6560df ("scsi: ipr: Remove SATA support") all > > libata drivers now have the error_handler() callback provided, > > so we can stop checking for non-existing error_handler callback. > > > > Signed-off-by: Hannes Reinecke <hare@suse.de> > > [niklas: fixed review comments, rebased, solved conflicts during rebase, > > fixed bug that unconditionally dumped all QCs, removed the now unused > > function ata_dump_status(), removed the now unreachable failure paths in > > atapi_qc_complete(), removed the non-EH function to request ATAPI sense] > > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com> Hello John, thank you for your review! > > ata_qc_from_tag() still has a ap->ops->error_handler check, right? Correct, I will remove the check from ata_qc_from_tag() as well. > > > --- > > drivers/ata/libata-core.c | 209 +++++++++++++++----------------------- > > drivers/ata/libata-eh.c | 150 ++++++++++++--------------- > > drivers/ata/libata-sata.c | 7 +- > > drivers/ata/libata-scsi.c | 142 ++------------------------ > > drivers/ata/libata-sff.c | 30 ++---- > > 5 files changed, 166 insertions(+), 372 deletions(-) > > > ... > > > > /* > > - * For new EH, all qcs are finished in one of three ways - > > + * For EH, all qcs are finished in one of three ways - > > * normal completion, error completion, and SCSI timeout. > > * Both completions can race against SCSI timeout. When normal > > * completion wins, the qc never reaches EH. When error > > @@ -659,94 +656,89 @@ EXPORT_SYMBOL(ata_scsi_cmd_error_handler); > > void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap) > > { > > unsigned long flags; > > + struct ata_link *link; > > /* invoke error handler */ > > Is this comment only really relevant when we may not previously invoked the > error handler? I'm not sure what you mean. I will simply drop this comment. Further down in this same function, we have: /* invoke EH, skip if unloading or suspended */ if (!(ap->pflags & (ATA_PFLAG_UNLOADING | ATA_PFLAG_SUSPENDED))) ap->ops->error_handler(ap); So a comment at the start of the function as well feels redundant. > > > - if (ap->ops->error_handler) { > > - struct ata_link *link; > > - /* acquire EH ownership */ > > - ata_eh_acquire(ap); > > + /* acquire EH ownership */ > > + ata_eh_acquire(ap); > > repeat: > > - /* kill fast drain timer */ > > - del_timer_sync(&ap->fastdrain_timer); > > + /* kill fast drain timer */ > > + del_timer_sync(&ap->fastdrain_timer); > > - /* process port resume request */ > > - ata_eh_handle_port_resume(ap); > > + /* process port resume request */ > > + ata_eh_handle_port_resume(ap); > > - /* fetch & clear EH info */ > > - spin_lock_irqsave(ap->lock, flags); > > + /* fetch & clear EH info */ > > + spin_lock_irqsave(ap->lock, flags); > > ... > > > * ata_to_sense_error - convert ATA error to SCSI error > > * @id: ATA device number > > @@ -904,7 +863,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > > struct ata_taskfile *tf = &qc->result_tf; > > unsigned char *sb = cmd->sense_buffer; > > unsigned char *desc = sb + 8; > > - int verbose = qc->ap->ops->error_handler == NULL; > > u8 sense_key, asc, ascq; > > memset(sb, 0, SCSI_SENSE_BUFFERSIZE); > > @@ -916,7 +874,7 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > > if (qc->err_mask || > > tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { > > ata_to_sense_error(qc->ap->print_id, tf->status, tf->error, > > - &sense_key, &asc, &ascq, verbose); > > + &sense_key, &asc, &ascq, false); > > ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq); > > } else { > > /* > > @@ -999,7 +957,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) > > struct scsi_cmnd *cmd = qc->scsicmd; > > struct ata_taskfile *tf = &qc->result_tf; > > unsigned char *sb = cmd->sense_buffer; > > - int verbose = qc->ap->ops->error_handler == NULL; > > u64 block; > > u8 sense_key, asc, ascq; > > @@ -1017,7 +974,7 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) > > if (qc->err_mask || > > tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { > > ata_to_sense_error(qc->ap->print_id, tf->status, tf->error, > > - &sense_key, &asc, &ascq, verbose); > > + &sense_key, &asc, &ascq, false); > > Please check this - AFAICS, we only ever pass false for @verbose arg now (so > it would not be needed, and ata_to_sense_error() may be simplified) You are correct, I will remove the parameter. Kind regards, Niklas > > > ata_scsi_set_sense(dev, cmd, sense_key, asc, ascq); > > } else { > > /* Could not decode error */ > > @@ -1179,9 +1136,6 @@ void ata_scsi_slave_destroy(struct scsi_device *sdev) > > unsigned long flags; > > struct ata_device *dev; > > - if (!ap->ops->error_handler) >
From: Niklas Cassel <niklas.cassel@wdc.com> Hi all, now that the ipr driver has been modified to not hook into libata all drivers now use the 'new' error handler, so we can remove any references to it. And do a general cleanup to remove callbacks which are no longer needed. Changes since v1: -Rebased against Damien's libata for-next branch. (Fixed conflicts in ata_qc_complete().) -Fixed comments from Damien. -Fixed comments from myself. -Do not dump all QCs unconditionally (which should make the performance regression reported by the test robot go away.) -Since ata_dump_status() was only called for drivers without .error_handler callback, this function is now unused and unreachable, so it was removed. -Since atapi_qc_complete() can no longer reach the "old EH" failure paths (code will now always take the top branch on error), they are removed. -Since atapi_eh_request_sense() can no longer be called, it is now removed. (The atapi_eh_request_sense() version called by EH is still there.) -Patch 7/8 and 8/8 are new in V2. Hannes Reinecke (6): ata: remove reference to non-existing error_handler() ata,scsi: remove ata_sas_port_{start,stop} callbacks ata,scsi: remove ata_sas_port_destroy() ata: remove ata_sas_sync_probe() ata: inline ata_port_probe() ata,scsi: cleanup ata_port_probe() Niklas Cassel (2): ata: sata_sx4: drop already completed TODO ata: remove ata_bus_probe() drivers/ata/libata-core.c | 355 +++++++---------------------- drivers/ata/libata-eh.c | 150 +++++------- drivers/ata/libata-sata.c | 77 ------- drivers/ata/libata-scsi.c | 142 +----------- drivers/ata/libata-sff.c | 30 +-- drivers/ata/libata.h | 3 - drivers/ata/sata_sx4.c | 1 - drivers/scsi/libsas/sas_ata.c | 6 +- drivers/scsi/libsas/sas_discover.c | 2 +- include/linux/libata.h | 6 +- 10 files changed, 170 insertions(+), 602 deletions(-)