On Fri, Feb 28, 2025 at 6:15 PM Tamir Duberstein <tamird@xxxxxxxxx> wrote:
>
> On Fri, Feb 28, 2025 at 12:08 PM Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
> >
> > On Fri, Feb 28, 2025 at 4:55 PM Tamir Duberstein <tamird@xxxxxxxxx> wrote:
> > >
> > > On Fri, Feb 28, 2025 at 7:41 AM Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
> > > >
> > > > @@ -980,8 +983,12 @@ fn draw_all(&mut self, data: impl Iterator<Item = u8>) {
> > > > /// * If `url_len` > 0, remove the 2 segments header/length and also count the
> > > > /// conversion to numeric segments.
> > > > /// * If `url_len` = 0, only removes 3 bytes for 1 binary segment.
> > > > -#[no_mangle]
> > > > -pub extern "C" fn drm_panic_qr_max_data_size(version: u8, url_len: usize) -> usize {
> > > > +///
> > > > +/// # Safety
> > > > +///
> > > > +/// Always safe to call.
> > >
> > > This should explain why it's marked unsafe, since it's always safe to call.
> >
> > Safety comments generally do not explain rationale for why they are
> > the way they are. Where would you like me to put it?
>
> Safety comments also generally do not say that the function isn't
> really unsafe (with a notable exception in
> `samples/rust/rust_print_main.rs` which is similar to this case).
>
> Perhaps "This function is marked unsafe because ... but since a safety
> comment is still required:" would flow nicely into the safety section.
I added a comment, but I disagree with this claim. The phrase "Always
safe to call." is actually quite common for a "# Safety" section, even
if we have rarely needed it in the kernel specifically.
> > > > @@ -36,6 +36,10 @@
> > > > #include <linux/workqueue.h>
> > > > #include <trace/events/rust_sample.h>
> > > >
> > > > +#if defined(CONFIG_DRM_PANIC_SCREEN_QR_CODE)
> > > > +#include <drm/drm_panic.h>
> > > > +#endif
> > >
> > > Why the guard here?
> > >
> > > It'd be nice to have a comment here explaining the atypical need for
> > > this include.
> >
> > It's not necessary. I can drop it.
>
> Ok. A comment on the include would still be helpful.
Added.
Alice
