| Message ID | 20240626230411.3471543-6-ipylypiv@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | ATA PASS-THROUGH sense data fixes | expand |
On Wed, Jun 26, 2024 at 11:04:10PM +0000, Igor Pylypiv wrote: > ATA_QCFLAG_RTF_FILLED is not specific to ahci and can be used generally > to check if qc->result_tf contains valid data. > > Reviewed-by: Hannes Reinecke <hare@suse.de> > Reviewed-by: Damien Le Moal <dlemoal@kernel.org> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > --- > drivers/ata/libahci.c | 10 ---------- > drivers/ata/libata-core.c | 8 ++++++++ > 2 files changed, 8 insertions(+), 10 deletions(-) > > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c > index 83431aae74d8..0728d445e531 100644 > --- a/drivers/ata/libahci.c > +++ b/drivers/ata/libahci.c > @@ -2075,13 +2075,6 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc) > struct ahci_port_priv *pp = qc->ap->private_data; > u8 *rx_fis = pp->rx_fis; > > - /* > - * rtf may already be filled (e.g. for successful NCQ commands). > - * If that is the case, we have nothing to do. > - */ > - if (qc->flags & ATA_QCFLAG_RTF_FILLED) > - return; > - > if (pp->fbs_enabled) > rx_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ; > > @@ -2095,7 +2088,6 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc) > !(qc->flags & ATA_QCFLAG_EH)) { > ata_tf_from_fis(rx_fis + RX_FIS_PIO_SETUP, &qc->result_tf); > qc->result_tf.status = (rx_fis + RX_FIS_PIO_SETUP)[15]; > - qc->flags |= ATA_QCFLAG_RTF_FILLED; > return; > } > > @@ -2118,12 +2110,10 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc) > */ > qc->result_tf.status = fis[2]; > qc->result_tf.error = fis[3]; > - qc->flags |= ATA_QCFLAG_RTF_FILLED; > return; > } > > ata_tf_from_fis(rx_fis + RX_FIS_D2H_REG, &qc->result_tf); > - qc->flags |= ATA_QCFLAG_RTF_FILLED; > } > > static void ahci_qc_ncq_fill_rtf(struct ata_port *ap, u64 done_mask) > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index e1bf8a19b3c8..a9fc3ec9300f 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -4801,8 +4801,16 @@ static void fill_result_tf(struct ata_queued_cmd *qc) > { > struct ata_port *ap = qc->ap; > > + /* > + * rtf may already be filled (e.g. for successful NCQ commands). > + * If that is the case, we have nothing to do. > + */ > + if (qc->flags & ATA_QCFLAG_RTF_FILLED) > + return; > + > qc->result_tf.flags = qc->tf.flags; One functional change that I can see from this is that after this commit, we will no longer do: qc->result_tf.flags = qc->tf.flags; if ATA_QCFLAG_RTF_FILLED was set. e.g. ata_scsi_set_passthru_sense_fields() and ata_gen_ata_sense() makes use of result_tf->flags, so we probably still want to do this. Perhaps keep this function as you have it and simply do: diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c index 0728d445e531..fdfa7b266218 100644 --- a/drivers/ata/libahci.c +++ b/drivers/ata/libahci.c @@ -2148,6 +2148,7 @@ static void ahci_qc_ncq_fill_rtf(struct ata_port *ap, u64 done_mask) if (qc && ata_is_ncq(qc->tf.protocol)) { qc->result_tf.status = status; qc->result_tf.error = error; + qc->result_tf.flags = qc->tf.flags; qc->flags |= ATA_QCFLAG_RTF_FILLED; } done_mask &= ~(1ULL << tag); @@ -2172,6 +2173,7 @@ static void ahci_qc_ncq_fill_rtf(struct ata_port *ap, u64 done_mask) fis += RX_FIS_SDB; qc->result_tf.status = fis[2]; qc->result_tf.error = fis[3]; + qc->result_tf.flags = qc->tf.flags; qc->flags |= ATA_QCFLAG_RTF_FILLED; } done_mask &= ~(1ULL << tag); > ap->ops->qc_fill_rtf(qc); > + qc->flags |= ATA_QCFLAG_RTF_FILLED; > } > > static void ata_verify_xfer(struct ata_queued_cmd *qc) > -- > 2.45.2.803.g4e1b14247a-goog >
On Fri, Jun 28, 2024 at 10:12:19PM +0200, Niklas Cassel wrote: > On Wed, Jun 26, 2024 at 11:04:10PM +0000, Igor Pylypiv wrote: > > ATA_QCFLAG_RTF_FILLED is not specific to ahci and can be used generally > > to check if qc->result_tf contains valid data. > > > > Reviewed-by: Hannes Reinecke <hare@suse.de> > > Reviewed-by: Damien Le Moal <dlemoal@kernel.org> > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > > --- > > drivers/ata/libahci.c | 10 ---------- > > drivers/ata/libata-core.c | 8 ++++++++ > > 2 files changed, 8 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c > > index 83431aae74d8..0728d445e531 100644 > > --- a/drivers/ata/libahci.c > > +++ b/drivers/ata/libahci.c > > @@ -2075,13 +2075,6 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc) > > struct ahci_port_priv *pp = qc->ap->private_data; > > u8 *rx_fis = pp->rx_fis; > > > > - /* > > - * rtf may already be filled (e.g. for successful NCQ commands). > > - * If that is the case, we have nothing to do. > > - */ > > - if (qc->flags & ATA_QCFLAG_RTF_FILLED) > > - return; > > - > > if (pp->fbs_enabled) > > rx_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ; > > > > @@ -2095,7 +2088,6 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc) > > !(qc->flags & ATA_QCFLAG_EH)) { > > ata_tf_from_fis(rx_fis + RX_FIS_PIO_SETUP, &qc->result_tf); > > qc->result_tf.status = (rx_fis + RX_FIS_PIO_SETUP)[15]; > > - qc->flags |= ATA_QCFLAG_RTF_FILLED; > > return; > > } > > > > @@ -2118,12 +2110,10 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc) > > */ > > qc->result_tf.status = fis[2]; > > qc->result_tf.error = fis[3]; > > - qc->flags |= ATA_QCFLAG_RTF_FILLED; > > return; > > } > > > > ata_tf_from_fis(rx_fis + RX_FIS_D2H_REG, &qc->result_tf); > > - qc->flags |= ATA_QCFLAG_RTF_FILLED; > > } > > > > static void ahci_qc_ncq_fill_rtf(struct ata_port *ap, u64 done_mask) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > > index e1bf8a19b3c8..a9fc3ec9300f 100644 > > --- a/drivers/ata/libata-core.c > > +++ b/drivers/ata/libata-core.c > > @@ -4801,8 +4801,16 @@ static void fill_result_tf(struct ata_queued_cmd *qc) > > { > > struct ata_port *ap = qc->ap; > > > > + /* > > + * rtf may already be filled (e.g. for successful NCQ commands). > > + * If that is the case, we have nothing to do. > > + */ > > + if (qc->flags & ATA_QCFLAG_RTF_FILLED) > > + return; > > + > > qc->result_tf.flags = qc->tf.flags; > > One functional change that I can see from this is that after this commit, > we will no longer do: qc->result_tf.flags = qc->tf.flags; > if ATA_QCFLAG_RTF_FILLED was set. Nice catch, Niklas! I'll fix it in v4. Thank you! > > e.g. ata_scsi_set_passthru_sense_fields() and ata_gen_ata_sense() > makes use of result_tf->flags, so we probably still want to do this. > > > Perhaps keep this function as you have it and simply do: > > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c > index 0728d445e531..fdfa7b266218 100644 > --- a/drivers/ata/libahci.c > +++ b/drivers/ata/libahci.c > @@ -2148,6 +2148,7 @@ static void ahci_qc_ncq_fill_rtf(struct ata_port *ap, u64 done_mask) > if (qc && ata_is_ncq(qc->tf.protocol)) { > qc->result_tf.status = status; > qc->result_tf.error = error; > + qc->result_tf.flags = qc->tf.flags; > qc->flags |= ATA_QCFLAG_RTF_FILLED; > } > done_mask &= ~(1ULL << tag); > @@ -2172,6 +2173,7 @@ static void ahci_qc_ncq_fill_rtf(struct ata_port *ap, u64 done_mask) > fis += RX_FIS_SDB; > qc->result_tf.status = fis[2]; > qc->result_tf.error = fis[3]; > + qc->result_tf.flags = qc->tf.flags; > qc->flags |= ATA_QCFLAG_RTF_FILLED; > } > done_mask &= ~(1ULL << tag); > > > > > ap->ops->qc_fill_rtf(qc); > > + qc->flags |= ATA_QCFLAG_RTF_FILLED; > > } > > > > static void ata_verify_xfer(struct ata_queued_cmd *qc) > > -- > > 2.45.2.803.g4e1b14247a-goog > >
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c index 83431aae74d8..0728d445e531 100644 --- a/drivers/ata/libahci.c +++ b/drivers/ata/libahci.c @@ -2075,13 +2075,6 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc) struct ahci_port_priv *pp = qc->ap->private_data; u8 *rx_fis = pp->rx_fis; - /* - * rtf may already be filled (e.g. for successful NCQ commands). - * If that is the case, we have nothing to do. - */ - if (qc->flags & ATA_QCFLAG_RTF_FILLED) - return; - if (pp->fbs_enabled) rx_fis += qc->dev->link->pmp * AHCI_RX_FIS_SZ; @@ -2095,7 +2088,6 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc) !(qc->flags & ATA_QCFLAG_EH)) { ata_tf_from_fis(rx_fis + RX_FIS_PIO_SETUP, &qc->result_tf); qc->result_tf.status = (rx_fis + RX_FIS_PIO_SETUP)[15]; - qc->flags |= ATA_QCFLAG_RTF_FILLED; return; } @@ -2118,12 +2110,10 @@ static void ahci_qc_fill_rtf(struct ata_queued_cmd *qc) */ qc->result_tf.status = fis[2]; qc->result_tf.error = fis[3]; - qc->flags |= ATA_QCFLAG_RTF_FILLED; return; } ata_tf_from_fis(rx_fis + RX_FIS_D2H_REG, &qc->result_tf); - qc->flags |= ATA_QCFLAG_RTF_FILLED; } static void ahci_qc_ncq_fill_rtf(struct ata_port *ap, u64 done_mask) diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index e1bf8a19b3c8..a9fc3ec9300f 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -4801,8 +4801,16 @@ static void fill_result_tf(struct ata_queued_cmd *qc) { struct ata_port *ap = qc->ap; + /* + * rtf may already be filled (e.g. for successful NCQ commands). + * If that is the case, we have nothing to do. + */ + if (qc->flags & ATA_QCFLAG_RTF_FILLED) + return; + qc->result_tf.flags = qc->tf.flags; ap->ops->qc_fill_rtf(qc); + qc->flags |= ATA_QCFLAG_RTF_FILLED; } static void ata_verify_xfer(struct ata_queued_cmd *qc)