| Message ID | 20240614191835.3056153-2-ipylypiv@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | ATA PASS-THROUGH sense data fixes | expand |
On 6/15/24 04:18, Igor Pylypiv wrote: > scsi_queue_rq() memsets sense_buffer before a command is dispatched. > > Libata is not memsetting sense_buffer before setting sense data that > was obtained from a disk so there should be no reason to do a memset > for ATA PASS-THROUGH / ATAPI. This sentence is not very clear at all... I assume that the first part of the sentence is for non passthrough commands. In this case, libata does not clear the sense buffer because the scsi layer did that already, in scsi_queue_rq() as noted above. For passthrough commands, the same should be happening as well since passthrough commands are also executed through blk_execute_rq() -> scsi_queue_rq(). So I do not really understand (but I do agree that the memset() in libata seem useless). > Memsetting the sense_buffer in ata_gen_passthru_sense() is erasing valid > sense data that was previously obtained from a disk. A follow-up patch > will modify ata_gen_passthru_sense() to stop generating sense data based > on ATA status register bits if a valid sense data is already present. This fix should come first in the series, since that commit will likely need to go into current rc and cc-stable. And that will simplify this patch as well. > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > --- > drivers/ata/libata-eh.c | 2 -- > drivers/ata/libata-scsi.c | 4 ---- > 2 files changed, 6 deletions(-) > > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c > index 214b935c2ced..b5e05efe73f6 100644 > --- a/drivers/ata/libata-eh.c > +++ b/drivers/ata/libata-eh.c > @@ -1479,8 +1479,6 @@ unsigned int atapi_eh_request_sense(struct ata_device *dev, > struct ata_port *ap = dev->link->ap; > struct ata_taskfile tf; > > - memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE); > - > /* initialize sense_buf with the error register, > * for the case where they are -not- overwritten > */ > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index cdf29b178ddc..032cf11d0bcc 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -858,8 +858,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > unsigned char *desc = sb + 8; > u8 sense_key, asc, ascq; > > - memset(sb, 0, SCSI_SENSE_BUFFERSIZE); > - > /* > * Use ata_to_sense_error() to map status register bits > * onto sense key, asc & ascq. > @@ -953,8 +951,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) > u64 block; > u8 sense_key, asc, ascq; > > - memset(sb, 0, SCSI_SENSE_BUFFERSIZE); > - > if (ata_dev_disabled(dev)) { > /* Device disabled after error recovery */ > /* LOGICAL UNIT NOT READY, HARD RESET REQUIRED */
On Fri, Jun 14, 2024 at 07:18:32PM +0000, Igor Pylypiv wrote: > scsi_queue_rq() memsets sense_buffer before a command is dispatched. > > Libata is not memsetting sense_buffer before setting sense data that > was obtained from a disk so there should be no reason to do a memset > for ATA PASS-THROUGH / ATAPI. > > Memsetting the sense_buffer in ata_gen_passthru_sense() is erasing valid > sense data that was previously obtained from a disk. A follow-up patch > will modify ata_gen_passthru_sense() to stop generating sense data based > on ATA status register bits if a valid sense data is already present. > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > --- > drivers/ata/libata-eh.c | 2 -- > drivers/ata/libata-scsi.c | 4 ---- > 2 files changed, 6 deletions(-) > > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c > index 214b935c2ced..b5e05efe73f6 100644 > --- a/drivers/ata/libata-eh.c > +++ b/drivers/ata/libata-eh.c > @@ -1479,8 +1479,6 @@ unsigned int atapi_eh_request_sense(struct ata_device *dev, > struct ata_port *ap = dev->link->ap; > struct ata_taskfile tf; > > - memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE); > - Are you sure that this is safe? atapi_eh_request_sense() is called both by: ata_eh_analyze_tf(): tmp = atapi_eh_request_sense(.., qc->scsicmd->sense_buffer, ..) and by: atapi_eh_clear_ua(): atapi_eh_request_sense(.., sense_buffer, ..); where sense_buffer is dev->link->ap->sector_buf. Wouldn't a better fix be for ata_gen_* functions to return early if ATA_QCFLAG_SENSE_VALID is set? > /* initialize sense_buf with the error register, > * for the case where they are -not- overwritten > */ > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index cdf29b178ddc..032cf11d0bcc 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -858,8 +858,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > unsigned char *desc = sb + 8; > u8 sense_key, asc, ascq; > > - memset(sb, 0, SCSI_SENSE_BUFFERSIZE); > - > /* > * Use ata_to_sense_error() to map status register bits > * onto sense key, asc & ascq. > @@ -953,8 +951,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) > u64 block; > u8 sense_key, asc, ascq; > > - memset(sb, 0, SCSI_SENSE_BUFFERSIZE); > - > if (ata_dev_disabled(dev)) { > /* Device disabled after error recovery */ > /* LOGICAL UNIT NOT READY, HARD RESET REQUIRED */ > -- > 2.45.2.627.g7a2c4fd464-goog >
On Mon, Jun 17, 2024 at 08:13:51AM +0900, Damien Le Moal wrote: > On 6/15/24 04:18, Igor Pylypiv wrote: > > scsi_queue_rq() memsets sense_buffer before a command is dispatched. > > > > Libata is not memsetting sense_buffer before setting sense data that > > was obtained from a disk so there should be no reason to do a memset > > for ATA PASS-THROUGH / ATAPI. > > This sentence is not very clear at all... I assume that the first part of the > sentence is for non passthrough commands. In this case, libata does not clear > the sense buffer because the scsi layer did that already, in scsi_queue_rq() as > noted above. > > For passthrough commands, the same should be happening as well since passthrough > commands are also executed through blk_execute_rq() -> scsi_queue_rq(). So I do > not really understand (but I do agree that the memset() in libata seem useless). Thanks! I'll update the commit message to make it more clear. > > > Memsetting the sense_buffer in ata_gen_passthru_sense() is erasing valid > > sense data that was previously obtained from a disk. A follow-up patch > > will modify ata_gen_passthru_sense() to stop generating sense data based > > on ATA status register bits if a valid sense data is already present. > > This fix should come first in the series, since that commit will likely need to > go into current rc and cc-stable. And that will simplify this patch as well. > Ack. I'll reorder the commits. > > > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > > --- > > drivers/ata/libata-eh.c | 2 -- > > drivers/ata/libata-scsi.c | 4 ---- > > 2 files changed, 6 deletions(-) > > > > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c > > index 214b935c2ced..b5e05efe73f6 100644 > > --- a/drivers/ata/libata-eh.c > > +++ b/drivers/ata/libata-eh.c > > @@ -1479,8 +1479,6 @@ unsigned int atapi_eh_request_sense(struct ata_device *dev, > > struct ata_port *ap = dev->link->ap; > > struct ata_taskfile tf; > > > > - memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE); > > - > > /* initialize sense_buf with the error register, > > * for the case where they are -not- overwritten > > */ > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > > index cdf29b178ddc..032cf11d0bcc 100644 > > --- a/drivers/ata/libata-scsi.c > > +++ b/drivers/ata/libata-scsi.c > > @@ -858,8 +858,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > > unsigned char *desc = sb + 8; > > u8 sense_key, asc, ascq; > > > > - memset(sb, 0, SCSI_SENSE_BUFFERSIZE); > > - > > /* > > * Use ata_to_sense_error() to map status register bits > > * onto sense key, asc & ascq. > > @@ -953,8 +951,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) > > u64 block; > > u8 sense_key, asc, ascq; > > > > - memset(sb, 0, SCSI_SENSE_BUFFERSIZE); > > - > > if (ata_dev_disabled(dev)) { > > /* Device disabled after error recovery */ > > /* LOGICAL UNIT NOT READY, HARD RESET REQUIRED */ > > -- > Damien Le Moal > Western Digital Research > Thank you, Igor
On Mon, Jun 17, 2024 at 12:41:26PM +0200, Niklas Cassel wrote: > On Fri, Jun 14, 2024 at 07:18:32PM +0000, Igor Pylypiv wrote: > > scsi_queue_rq() memsets sense_buffer before a command is dispatched. > > > > Libata is not memsetting sense_buffer before setting sense data that > > was obtained from a disk so there should be no reason to do a memset > > for ATA PASS-THROUGH / ATAPI. > > > > Memsetting the sense_buffer in ata_gen_passthru_sense() is erasing valid > > sense data that was previously obtained from a disk. A follow-up patch > > will modify ata_gen_passthru_sense() to stop generating sense data based > > on ATA status register bits if a valid sense data is already present. > > > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > > --- > > drivers/ata/libata-eh.c | 2 -- > > drivers/ata/libata-scsi.c | 4 ---- > > 2 files changed, 6 deletions(-) > > > > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c > > index 214b935c2ced..b5e05efe73f6 100644 > > --- a/drivers/ata/libata-eh.c > > +++ b/drivers/ata/libata-eh.c > > @@ -1479,8 +1479,6 @@ unsigned int atapi_eh_request_sense(struct ata_device *dev, > > struct ata_port *ap = dev->link->ap; > > struct ata_taskfile tf; > > > > - memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE); > > - > > Are you sure that this is safe? > > atapi_eh_request_sense() is called both by: > ata_eh_analyze_tf(): > tmp = atapi_eh_request_sense(.., qc->scsicmd->sense_buffer, ..) > > and by: > atapi_eh_clear_ua(): > atapi_eh_request_sense(.., sense_buffer, ..); > where sense_buffer is dev->link->ap->sector_buf. > Thanks for pointing this out, Niklas! ata_eh_analyze_tf() case is safe because qc->scsicmd->sense_buffer is cleared by scsi_queue_rq(). atapi_eh_clear_ua() case is safe right now because the sense buffer contents are not being used. However, someone might start using the sense data in the future so it is not safe to leave it as-is. There's one more place where this function is being called: zpready(): atapi_eh_request_sense(..., sense_buf, ...); where sense_buffer is dev->link->ap->sector_buf. This one is actually using the obtained sense buffer so it would be a nasty bug if we don't do a memset(). I think we should explicitly memset buffers before passing them to atapi_eh_request_sense() in atapi_eh_clear_ua() and zpready() so that atapi_eh_request_sense() can have the same behavior as ata_eh_request_sense() with regards to sense buffer expectations i.e. both functions will expect buffers that are already memeset to zero. > > Wouldn't a better fix be for ata_gen_* functions to return early if > ATA_QCFLAG_SENSE_VALID is set? > It would be possible to return early if ATA_QCFLAG_SENSE_VALID is set once we factor out "ATA Status Return sense data descriptor" population out of ata_gen_passthru_sense() into a separate function. I'll factor out the descriptor population code in v2. I think that it is still benefitial to remove the redundant memset() from the ata_eh_analyze_tf() -> atapi_eh_request_sense() path? > > > /* initialize sense_buf with the error register, > > * for the case where they are -not- overwritten > > */ > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > > index cdf29b178ddc..032cf11d0bcc 100644 > > --- a/drivers/ata/libata-scsi.c > > +++ b/drivers/ata/libata-scsi.c > > @@ -858,8 +858,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > > unsigned char *desc = sb + 8; > > u8 sense_key, asc, ascq; > > > > - memset(sb, 0, SCSI_SENSE_BUFFERSIZE); > > - > > /* > > * Use ata_to_sense_error() to map status register bits > > * onto sense key, asc & ascq. > > @@ -953,8 +951,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) > > u64 block; > > u8 sense_key, asc, ascq; > > > > - memset(sb, 0, SCSI_SENSE_BUFFERSIZE); > > - > > if (ata_dev_disabled(dev)) { > > /* Device disabled after error recovery */ > > /* LOGICAL UNIT NOT READY, HARD RESET REQUIRED */ > > -- > > 2.45.2.627.g7a2c4fd464-goog > > Thank you, Igor
On Tue, Jun 18, 2024 at 07:58:56PM +0000, Igor Pylypiv wrote: > > I think we should explicitly memset buffers before passing them to > atapi_eh_request_sense() in atapi_eh_clear_ua() and zpready() so that > atapi_eh_request_sense() can have the same behavior as ata_eh_request_sense() > with regards to sense buffer expectations i.e. both functions will expect > buffers that are already memeset to zero. Well, you could argue that: static bool ata_eh_request_sense(struct ata_queued_cmd *qc) doesn't take a sense_buffer, but: unsigned int atapi_eh_request_sense(struct ata_device *dev, u8 *sense_buf, u8 dfl_sense_key) does, so it makes sense for atapi_eh_request_sense() to memset() the buffer. > I think that it is still benefitial to remove the redundant memset() from > the ata_eh_analyze_tf() -> atapi_eh_request_sense() path? atapi_eh_request_sense() should only be called when ATA_SENSE bit is set, so this is only called in special circumstances, so it is not like the memset() is in the hot path. If you ask me, I think that the current code is fine. Kind regards, Niklas
On Thu, Jun 20, 2024 at 01:51:26PM +0200, Niklas Cassel wrote: > On Tue, Jun 18, 2024 at 07:58:56PM +0000, Igor Pylypiv wrote: > > > > I think we should explicitly memset buffers before passing them to > > atapi_eh_request_sense() in atapi_eh_clear_ua() and zpready() so that > > atapi_eh_request_sense() can have the same behavior as ata_eh_request_sense() > > with regards to sense buffer expectations i.e. both functions will expect > > buffers that are already memeset to zero. > > Well, you could argue that: > static bool ata_eh_request_sense(struct ata_queued_cmd *qc) > doesn't take a sense_buffer, but: > > unsigned int atapi_eh_request_sense(struct ata_device *dev, > u8 *sense_buf, u8 dfl_sense_key) > > does, so it makes sense for atapi_eh_request_sense() to memset() the buffer. > > > > I think that it is still benefitial to remove the redundant memset() from > > the ata_eh_analyze_tf() -> atapi_eh_request_sense() path? > > atapi_eh_request_sense() should only be called when ATA_SENSE bit is set, > so this is only called in special circumstances, so it is not like the > memset() is in the hot path. > > If you ask me, I think that the current code is fine. > I didn't think about the "takes a sense_buffer as an argument" vs "doesn't take a sense_buffer as an argument" aspect. Yeah, keeping memset() makes sense in this case. I'll drop the memset removal from atapi_eh_request_sense() in v2. Thank you! Igor > > Kind regards, > Niklas
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index 214b935c2ced..b5e05efe73f6 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -1479,8 +1479,6 @@ unsigned int atapi_eh_request_sense(struct ata_device *dev, struct ata_port *ap = dev->link->ap; struct ata_taskfile tf; - memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE); - /* initialize sense_buf with the error register, * for the case where they are -not- overwritten */ diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index cdf29b178ddc..032cf11d0bcc 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -858,8 +858,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) unsigned char *desc = sb + 8; u8 sense_key, asc, ascq; - memset(sb, 0, SCSI_SENSE_BUFFERSIZE); - /* * Use ata_to_sense_error() to map status register bits * onto sense key, asc & ascq. @@ -953,8 +951,6 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc) u64 block; u8 sense_key, asc, ascq; - memset(sb, 0, SCSI_SENSE_BUFFERSIZE); - if (ata_dev_disabled(dev)) { /* Device disabled after error recovery */ /* LOGICAL UNIT NOT READY, HARD RESET REQUIRED */
scsi_queue_rq() memsets sense_buffer before a command is dispatched. Libata is not memsetting sense_buffer before setting sense data that was obtained from a disk so there should be no reason to do a memset for ATA PASS-THROUGH / ATAPI. Memsetting the sense_buffer in ata_gen_passthru_sense() is erasing valid sense data that was previously obtained from a disk. A follow-up patch will modify ata_gen_passthru_sense() to stop generating sense data based on ATA status register bits if a valid sense data is already present. Signed-off-by: Igor Pylypiv <ipylypiv@google.com> --- drivers/ata/libata-eh.c | 2 -- drivers/ata/libata-scsi.c | 4 ---- 2 files changed, 6 deletions(-)