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

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

Hi,

On Thu, Apr 23, 2026 at 06:53:12PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> Hello Takashi,
> 
> On Thu, Apr 23, 2026 at 11:19:59PM +0900, Takashi Sakamoto wrote:
> > It is unclear who is responsible for maintaining the 'ieee1394_device_id'
> > structure in include/linux/mod_devicetable.h, but if it falls under my
> > responsibility (which seems likely),
> 
> It matches my understanding that it's indeed you who is responsible for
> ieee1394_device_id.
> 
> > I would prefer to postpone applying these patches, or at least exclude
> > them from this merge window.
> 
> I don't expect them to go in for v7.1-rc1. My idea was to send this kind
> of patch during the merge window to allow to get it into next soon after
> the merge window closed to allow some cooking in next.
>  
> > After reading the discussions around the UAPI, I am not fully convinced
> > that your patches appear to provide clear benefits to existing
> > IEEE 1394 bus users or their software. From my perspective, the motivation
> > appears to be primarily related to the CHERI extension work.
> 
> Yes and no. My motivation to work on these is triggered by CHERI indeed.
> But I already worked on this kind of update even before I knew about
> CHERI to get rid of the casts. (Back then it was about i2c, see
> https://lore.kernel.org/all/20240426213832.915485-2-u.kleine-koenig@pengutronix.de/.)
> 
> For me this series is a sweet spot, because it allows to reduce the
> CHERI patch stack but at the same time reduces the amount of (IMHO ugly)
> casts involved in handling pointers in .driver_data. With my mainline
> maintainer hat on, I consider the latter alone a good enough reason to
> apply patches like this. One issue it addresses en passant is that a
> part of the casts back to a pointer drop the const attribute, see the
> output of
> 
> 	git grep '\*).*driver_data' | grep -v '\<const\>'
> 
> . I didn't check for false positives in this list, but I guess there are
> not many. This doesn't affect firewire though, there the casts are all
> fine.
> 
> As my patch series doesn't introduce changes to the compiled drivers,
> the effect on bus users or their software is admittedly quite limited of
> course :-), but I'd hope that you see a benefit in the 2nd patch even if
> CHERI isn't (and probably shouldn't be) a motivation for you.
> 
> The reason I mentioned CHERI was mainly to be transparent about my
> motivation, but this triggered more discussion than I liked, distracting
> from the advantages of the changes for non-CHERI archs.
> 
> > As you know, this subsystem is quite marginal in the Linux kernel
> > codebase. Given that, it might be worth considering whether this
> > subsystem should be excluded from the build target when capability
> > pointers are enabled (e.g. via Kconfig, if available), since it does not
> > appear to work outside the ILP32 or LP64 data models. It may be worth
> > carefully considering where effort is best spent. I can understand the
> > merits of CHERI extensions, but changes related to this subsystem would
> > likely be acceptable only after the kernel core functions have been
> > updated.
> 
> I wasn't aware about this limitation. struct ieee1394_device_id just
> happend to be near the top of include/linux/mod_devicetable.h. (There is
> only pci_device_id before, but I wanted to start with a smaller part of
> this quest.)
>  
> > That said, this is just my current view. I would welcome any feedback or
> > objections. Besides, it is still one of my tasks to figure out how to
> > adapt the UAPI structures and the firewire core implementations to
> > non-ILP32/LP64 data models.
> 
> With you talking much about UAPI I wonder if you're aware that for all
> current Linux architectures my patch set doesn't introduce any changes
> in the binary interface because all have sizeof(unsigned long) ==
> sizeof(union { unsigned long; const void *; }). And the two adaptions
> (my introduction of the union for driver_data + your UAPI structure
> adaptions) should not restrict each other, right?
> 
> So I'd be glad if you forgot about CHERI and just judge the patchset for
> the things it achieves for current mainline. Of course I hope you agree
> it's a nice cleanup eventually and apply it.

Setting aside CHERI-related considerations, what concrete benefits do
these patches provide to the current kernel? Are those benefits clearly
explained in the cover letter and the patch comments?

From the cover letter, I understand that:

> 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. 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).

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.


Thanks

Takashi Sakamoto