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
