On 11/07/2024 23:00, Alice Ryhl wrote:
On Wed, Jul 10, 2024 at 4:01 PM Jocelyn Falempe <jfalempe@xxxxxxxxxx> wrote:
This patch adds a new panic screen, with a QR code and the kmsg data
embedded.
If DRM_PANIC_SCREEN_QR_CODE_URL is set, then the kmsg data will be
compressed with zlib and encoded as a numerical segment, and appended
to the URL as a URL parameter. This allows to save space, and put
about ~7500 bytes of kmsg data, in a V40 QR code.
Linux distributions can customize the URL, and put a web frontend to
directly open a bug report with the kmsg data.
Otherwise the kmsg data will be encoded as a binary segment (ie raw
ascii) and only a maximum of 2953 bytes of kmsg data will be
available in the QR code.
You can also limit the QR code size with DRM_PANIC_SCREEN_QR_VERSION.
v2:
* Rewrite the rust comments with Markdown (Alice Ryhl)
* Mark drm_panic_qr_generate() as unsafe (Alice Ryhl)
* Use CStr directly, and remove the call to as_str_unchecked()
(Alice Ryhl)
* Add a check for data_len <= data_size (Greg KH)
v3:
* Fix all rust comments (typo, punctuation) (Miguel Ojeda)
* Change the wording of safety comments (Alice Ryhl)
* Add a link to the javascript decoder in the Kconfig (Greg KH)
* Fix data_size and tmp_size check in drm_panic_qr_generate()
Signed-off-by: Jocelyn Falempe <jfalempe@xxxxxxxxxx>
---
Overall, it looks reasonable to me. Some comments below.
The changelog should go below the --- or in the cover letter.
+ if (stream.total_out > max_qr_data_size) {
+ /* too much data for the QR code, so skip the first line and try again */
+ kmsg = strchr(kmsg + 1, '\n');
+ if (!kmsg)
+ return -EINVAL;
+ kmsg_len = strlen(kmsg);
+ goto try_again;
It seems like kmsg will start with a newline character when this retry
routine runs. Is that really what you want? Why not instead strchr and
then do the plus one afterwards?
Good catch, yes I should increment kmsg after the strchr, to skip the
'\n' character.
This would also simplify the logic for why `kmsg + 1` doesn't go out
of bounds. Right now I have to check that there's no codepath where
kmsg points at the nul terminator byte.
sure, I will do that for v4.
+const __LOG_PREFIX: &[u8] = b"rust_qrcode\0";
I guess this constant is because you're not using the module! macro?
I think it's a leftover of when I used pr_info!() to debug my rust code.
Drm panic is built within the main drm module, and is not a module on
its own, so I'm not sure if I can use the module! macro here.
+/// C entry point for the rust QR Code generator.
+///
+/// Write the QR code image in the data buffer, and return the QR code width,
+/// or 0, if the data doesn't fit in a QR code.
+///
+/// * `url`: The base URL of the QR code. It will be encoded as Binary segment.
+/// * `data`: A pointer to the binary data, to be encoded. if URL is NULL, it
+/// will be encoded as binary segment, otherwise it will be encoded
+/// efficiently as a numeric segment, and appended to the URL.
+/// * `data_len`: Length of the data, that needs to be encoded, must be less
+/// than data_size.
+/// * `data_size`: Size of data buffer, it should be at least 4071 bytes to hold
+/// a V40 QR code. It will then be overwritten with the QR code image.
+/// * `tmp`: A temporary buffer that the QR code encoder will use, to write the
+/// segments and ECC.
+/// * `tmp_size`: Size of the temporary buffer, it must be at least 3706 bytes
+/// long for V40.
+///
+/// # Safety
+///
+/// * `url` must be null or point at a nul-terminated string.
+/// * `data` must be valid for reading and writing for `data_size` bytes.
+/// * `tmp` must be valid for reading and writing for `tmp_size` bytes.
+///
+/// They must remain valid for the duration of the function call.
+
+#[no_mangle]
+pub unsafe extern "C" fn drm_panic_qr_generate(
+ url: *const i8,
+ data: *mut u8,
+ data_len: usize,
+ data_size: usize,
+ tmp: *mut u8,
+ tmp_size: usize,
+) -> u8 {
+ if data_size < 4071 || tmp_size < 3706 || data_len > data_size {
+ return 0;
+ }
+ // SAFETY: The caller ensures that `data` is a valid pointer for reading and
+ // writing `data_size` bytes.
+ let data_slice: &mut [u8] = unsafe { core::slice::from_raw_parts_mut(data, data_size) };
+ // SAFETY: The caller ensures that `tmp` is a valid pointer for reading and
+ // writing `tmp_size` bytes.
+ let tmp_slice: &mut [u8] = unsafe { core::slice::from_raw_parts_mut(tmp, tmp_size) };
+ if url.is_null() {
+ match EncodedMsg::new(&[&Segment::Binary(&data_slice[0..data_len])], tmp_slice) {
+ None => 0,
+ Some(em) => {
+ let qr_image = QrImage::new(&em, data_slice);
+ qr_image.width
+ }
+ }
+ } else {
+ // SAFETY: The caller ensures that `url` is a valid pointer to a
+ // nul-terminated string.
+ let url_cstr: &CStr = unsafe { CStr::from_char_ptr(url) };
+ let segments = &[
+ &Segment::Binary(url_cstr.as_bytes()),
+ &Segment::Numeric(&data_slice[0..data_len]),
+ ];
+ match EncodedMsg::new(segments, tmp_slice) {
+ None => 0,
+ Some(em) => {
+ let qr_image = QrImage::new(&em, data_slice);
+ qr_image.width
+ }
+ }
+ }
+}
This looks good to me. :)
Thanks a lot
Alice
--
Jocelyn
