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: 4100 bytes --]

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.

Best regards
Uwe

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