Re: [PATCH] cdrom: modernize locking, memory allocation, and fix checkpatch warnings

[Phillip Potter]

On Tue, Jun 02, 2026 at 05:07:18PM +0200, Jedrzej-Sz wrote:
> From: Jędrzej Szoszorek <jedrzej.szoszo@xxxxxxxxx>
> 
> This patch introduces several modernizations and fixes to the legacy cdrom driver to align it with current kernel standards:
> 
>     Concurrency: Added cdrom_events_lock spinlock to protect vfs_events and ioctl_events from data races. Wrapped use_count modifications in cdrom_open() and cdrom_release() with cdrom_mutex.
> 
>     Memory Safety: Replaced obsolete and unsafe allocation methods with kzalloc() and struct_size() to prevent potential buffer overflows when allocating flexible arrays (e.g., struct cdrom_changer_info).
> 
>     Sysctl API: Refactored cdrom_sysctl_info() to use dynamic memory allocation for the info buffer instead of a hardcoded string array, preventing truncation with multiple drives. Additionally, removed the empty {} sentinel at the end of cdrom_table to fix runtime warnings with the modern register_sysctl API.
> 
>     Error Handling: Replaced inappropriate -ENOSYS returns in ioctl paths with -ENOTTY, as expected by current kernel design patterns.
> 
>     Includes: Migrated from <asm/unaligned.h> to <linux/unaligned.h> to resolve compilation errors on modern mainline trees, updating unaligned memory access macros accordingly.
> 
>     Code Style: Fixed numerous formatting issues, block comment alignments, trailing whitespaces, and replaced hardcoded function names in debug logs with __func__ to ensure checkpatch.pl compliance.
> 
> Signed-off-by: Jędrzej Szoszorek <jedrzej.szoszo@xxxxxxxxx>
> ---
> Note: During the review and refactoring of this legacy driver, I used 
> an AI assistant (Gemini) to help identify outdated API patterns (like 
> missing event locks, legacy allocations, and sysctl sentinels). All 
> suggestions were manually audited, verified against current kernel APIs, 
> and successfully tested in a minimal QEMU environment.
> 

Dear Jędrzej,

Thank you for the patch. In its current form however, I don't feel like
it's something I can effectively review. Few comments from me:

(1) This is a huge patch. 700+ insertions and 600+ removals. If you're
    serious about the changes, please split them up.

(2) I'm not convinced of the value of the changes, for example just
    reformatting the contributors list. It doesn't add much value as far
    as I can see, whilst of course being structurally correct. It is
    'churn' technically speaking.

(3) If you are changing error values and other such things, I'm curious
    to know how these changes have been tested, and if they introduce
    regressions in calling code? When this is such a big refactor,
    "successfully tested in a minimal QEMU environment." doesn't cut it
    for me at present.

Given the legacy/widespread nature of the driver, whilst I'm not
explicitly anti-AI, I would hope that all these changes are well
understood. In particular, that they solve real problems/issues without
introducing regressions. If that's the case, then I'm happy to take a
look at a slimmed down and revised set of patches that effect equivalent
changes to the driver.

All the best,
Phil