Message-ID: <CANiq72kS2fAgRnR8yNfpN69tMG+UPfgfytaA8sE=tYH+OQ_L6A@mail.gmail.com>
Date: Tue, 9 Jul 2024 11:41:06 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: Jocelyn Falempe <jfalempe@...hat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, 
	Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Wedson Almeida Filho <wedsonaf@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Bjorn Roy Baron <bjorn3_gh@...tonmail.com>, Benno Lossin <benno.lossin@...ton.me>, 
	Andreas Hindborg <a.hindborg@...sung.com>, Alice Ryhl <aliceryhl@...gle.com>, 
	linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org, 
	rust-for-linux@...r.kernel.org, Danilo Krummrich <dakr@...hat.com>
Subject: Re: [PATCH v2 4/4] drm/panic: Add a qr_code panic screen

Hi Jocelyn,

A quick docs-only review of the Rust side (some of these apply in
several cases -- I just wanted to give an overview for you to
consider).

On Tue, Jul 9, 2024 at 10:45 AM Jocelyn Falempe <jfalempe@...hat.com> wrote:
>
> +//! This is a simple qr encoder for DRM panic.
> +//!
> +//! Due to the Panic constraint, it doesn't allocate memory and does all

Perhaps clarify "Panic constraint" here?

> +//! the work on the stack or on the provided buffers. For
> +//! simplification, it only supports Low error correction, and apply the

"applies"?

> +//! first mask (checkboard). It will draw the smallest QRcode that can

"QR code"? "QR-code"?

In other places "QR-code" is used -- it would be ideal to be
consistent. (Although, isn't the common spelling "QR code"?)

> +//! contain the string passed as parameter. To get the most compact
> +//! QR-code, the start of the url is encoded as binary, and the

Probably "URL".

> +//! compressed kmsg is encoded as numeric.
> +//!
> +//! The binary data must be a valid url parameter, so the easiest way is
> +//! to use base64 encoding. But this waste 25% of data space, so the

"wastes"

> +//! whole stack trace won't fit in the QR-Code. So instead it encodes
> +//! every 13bits of input into 4 decimal digits, and then use the

"uses"

> +//! efficient numeric encoding, that encode 3 decimal digits into
> +//! 10bits. This makes 39bits of compressed data into 12 decimal digits,
> +//! into 40bits in the QR-Code, so wasting only 2.5%. And numbers are
> +//! valid url parameter, so the website can do the reverse, to get the

"And the numbers are valid URL parameters"?

> +//! Inspired by this 3 projects, all under MIT license:

"these"

> +// Generator polynomials for QR Code, only those that are needed for Low quality

If possible, please remember to use periods at the end for both
comments and docs. It is very pedantic, but if possible we would like
to try to be consistent across subsystems on how the documentation
looks etc. If everything looks the same, it is also easy to
remember/check how to do it for new files and so on.

> +/// QRCode parameter for Low quality ECC:
> +/// - Error Correction polynomial
> +/// - Number of blocks in group 1
> +/// - Number of blocks in group 2
> +/// - Block size in group 1
> +/// (Block size in group 2 is one more than group 1)

We typically leave a newline after a list.

> +    // Return the smallest QR Version than can hold these segments
> +    fn from_segments(segments: &[&Segment<'_>]) -> Option<Version> {

Should be docs, even if private? i.e. `///`?

Also third person and period.

> +// padding bytes
> +const PADDING: [u8; 2] = [236, 17];

`///`?

> +/// get the next 13 bits of data, starting at specified offset (in bits)

Please capitalize.

> +        // b is 20 at max (bit_off <= 7 and size <= 13)

Please use Markdown for comments too.

> +/// EncodedMsg will hold the data to be put in the QR-Code, with correct segment
> +/// encoding, padding, and Error Code Correction.

Missing newline? In addition, for the title (i.e. first paragraph), we
try to keep it short/simple, e.g. you could perhaps say something
like:

    /// Data to be put in the QR code (with correct segment encoding,
padding, and error code correction).

> +/// QrImage
> +///
> +/// A QR-Code image, encoded as a linear binary framebuffer.

Please remove the title -- the second paragraph should be the title.

> +/// Max width is 177 for V40 QR code, so u8 is enough for coordinate.

`u8`

> +/// drm_panic_qr_generate()

You can remove this title.

> +/// C entry point for the rust QR Code generator.
> +///
> +/// Write the QR code image in the data buffer, and return the qrcode size, 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.

Typically we would write a colon. after the key, e.g.

    /// * `url`: the base URL of the QR code.

> +/// # Safety
> +///
> +/// * `url` must be null or point at a nul-terminated string.
> +/// * `data` must be valid for reading and writing for `data_size` bytes.
> +/// * `data_len` must be less than `data_size`.
> +/// * `tmp` must be valid for reading and writing for `tmp_size` bytes.

It would be nice to mention for which duration these need to hold,
e.g. the call or something else.

> +        // Safety: url must be a valid pointer to a nul-terminated string.

Please use the `// SAFETY: ` prefix instead, since it is how we tag
these (i.e. differently from from the `# Safety` section).

> +/// * `version` QR code version, between 1-40.

If something like this happens to be used in several places, you may
want to consider using transparent newtypes for them. This would allow
you to avoid having to document each use point and it would enrich the
signatures.

Thanks!

Cheers,
Miguel

Powered by blists - more mailing lists