Skip to content

Conversation

@sc108-lee
Copy link
Contributor

if rae default is true, it's always true
so it was not possible to issue with RAE=0

If the host is reading the Telemetry Controller-Initiated log page, then the host reads any portion of that log page with the Retain Asynchronous Event bit cleared to ‘0’ to indicate to the controller that the host has completed reading the Telemetry Controller-Initiated log page

if rae default is true, it's always true
so it was not possible to issue with RAE=0

If the host is reading the Telemetry Controller-Initiated log page,
then the host reads any portion of that log page with the Retain
Asynchronous Event bit cleared to ‘0’ to indicate to the controller
that the host has completed reading the Telemetry Controller-Initiated
log page

Signed-off-by: Steven Seungcheol Lee <[email protected]>
@igaw
Copy link
Collaborator

igaw commented Dec 2, 2024

1af3b3d ("nvme: Support telemetry-log over nvme-mi") had the comment

set rae = true so it won't clear the current telemetry log in controller

If I understand your change, this would clear the entry. Sounds like two different use cases. Do we need to have a --rae for this command with a sane default?

@sc108-lee
Copy link
Contributor Author

sc108-lee commented Dec 3, 2024

set rae = true so it won't clear the current telemetry log in controller

That was only for reading telemetry header to check Telemetry Controller-Initiated Data Available ( telem->ctrlavail )

Before gathering actual ctrl telemetry log, it should not clear the entry, that's why rae=1
But under logic (gathering actual ctrl telemetry log), should be possible to select rae field.

nvme_get_log_telemetry_ctrl in libnvme (ioctl.h)
-> call nvme_get_log_page

nvme_get_log_page set rae with below logic
args->rae = offset + xfer < data_len || retain; ( if rae is given, always true, if there is more log size needed then true)

so in telemetry, we need the logic
rae : true -> no change (currently always true(1)) -> not clear
read telemetry header with rae=1
read telemetry with get_log (since retain=1, rae always true)

rae : false -> we can clear after collecting ctrl init telemetry log.
read telemetry header with rae=1
read telemetry with get_log (since retain=0, rae will be 0 when last log_page issued)

it could change user experience, but still need to be fixed
since spec says
"If the host is reading the Telemetry Controller-Initiated log page, then the host reads any portion of that log page with the Retain Asynchronous Event bit cleared to ‘0’ to indicate to the controller that the host has completed reading the Telemetry Controller-Initiated log page"

@igaw
Copy link
Collaborator

igaw commented Dec 3, 2024

Thanks a lot for digging into it. We had plenty of rae changes and I am getting a bit nervous if we touch this area again. So it's good to have a complete analysis why this is correct.

@igaw igaw merged commit 4e7327f into linux-nvme:master Dec 3, 2024
17 checks passed
@sc108-lee
Copy link
Contributor Author

If I may add one thing for the record,
In NVM-Express-Management-Interface-Specification-Revision-2.0-2024.08.05-Ratified.pdf
Figure 134: List of NVMe Admin Commands Supported using the Out-of-Band Mechanism
-> 2 The Management Endpoint shall ignore the Retain Asynchronous Event (RAE) bit in the Get Log Page command (refer to the NVM Express Base Specification). If the RAE bit is supported and the log page is used with Asynchronous Events, then the Get Log Page command shall be processed as if the RAE bit is set to ‘1’.

summary
MI Endpoint should ignore this bit, and work as RAE is 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants