Re: [PATCH v1 0/2] firewire: Simplify storing pointers in device id struct

["Uwe Kleine-König (The Capable Hub)" <u.kleine-koenig@baylibre.com>]

[-- Attachment #1: Type: text/plain, Size: 2306 bytes --]

Hello Takashi,

On Mon, Apr 27, 2026 at 03:37:19PM +0900, Takashi Sakamoto wrote:
> On Thu, Apr 23, 2026 at 06:53:12PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> > A considerable amount of drivers for the first category uses the
> > unsigned long variable to store a pointer. This involves casting both
> > for assignment and usage.
> 
> The conversion between 'unsigned long' and 'void *' are not inherently
> invalid within ILP32/LP64 data models.

Agreed, it's not invalid. But casts are always a part in C code where
strange or even bad things can happen as the (weak) type-safety of C is
overridden. Arguably there is still casting involved as driver_data_ptr
is a void pointer, but this is near enough to the actually type of the
driver data to not require an explicit cast. In my eyes reducing the
number of casts is a maintenance improvement. That's a bit similar to
reducing the number of unsafe parts in Rust code. It reduces the amount
of code you have to keep an eye on when thinking about correctness of
the code and it makes drivers suspicious that use both .driver_data and
.driver_data_ptr (which is easy to grep for).

> As I see it, the main significant
> effect of these patches is to add a 'const' qualifier to pointer values
> (and improve type safety, presumably in your view).

For me the main benefit is that the explicit casts can go away. This is
what is implemented in the 2nd patch with changes like

-               detect_formats = (snd_dice_detect_formats_t)entry->driver_data;
+               detect_formats = entry->driver_data_ptr;

but yes, that the const is kept this way is also a nice upside. I'm
currently working on a similar patch set for pci_device_id and this
indeed shows a few missing `const`s.

> If that is the case, why do the patch comments spend so much lines
> describing the size of unsigned long and pointer types, and CHERI
> extension? These points do not seem directly relevant to the practical
> benefits described above.
> 
> It would be better to repost the patches with clearer and more focused
> comments so that subsystem maintainers are more likely to apply them
> without the background discussion about the CHERI extension.

I'll try that. Thanks for your feedback.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]