Re: [PATCH RFC RESEND] soc: pxa: ssp: Cast to enum pxa_ssp_type instead of int

[Uwe Kleine-König]

Hello Arnd,

On Thu, Jan 04, 2024 at 11:03:41AM +0100, Arnd Bergmann wrote:
> On Thu, Jan 4, 2024, at 10:08, Uwe Kleine-König wrote:
> > [adding lakml to Cc for wider audience]
> >
> > On Wed, Jan 03, 2024 at 10:06:03PM +0100, Duje Mihanović wrote:
> >> On ARM64 platforms, id->data is a 64-bit value and casting it to a
> >> 32-bit integer causes build errors. Cast it to the corresponding enum
> >> instead.
> >> 
> >> Signed-off-by: Duje Mihanović <duje.mihanovic@xxxxxxxx>
> >> ---
> >> This patch is necessary for my Marvell PXA1908 series to compile successfully
> >> with allyesconfig:
> >> https://lore.kernel.org/all/20231102-pxa1908-lkml-v7-0-cabb1a0cb52b@xxxxxxxx/
> >> ---
> >>  drivers/soc/pxa/ssp.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/soc/pxa/ssp.c b/drivers/soc/pxa/ssp.c
> >> index a1e8a07f7275..e2ffd8fd7e13 100644
> >> --- a/drivers/soc/pxa/ssp.c
> >> +++ b/drivers/soc/pxa/ssp.c
> >> @@ -152,11 +152,11 @@ static int pxa_ssp_probe(struct platform_device *pdev)
> >>  	if (dev->of_node) {
> >>  		const struct of_device_id *id =
> >>  			of_match_device(of_match_ptr(pxa_ssp_of_ids), dev);
> >> -		ssp->type = (int) id->data;
> >> +		ssp->type = (enum pxa_ssp_type) id->data;
> >
> > I wonder if this is a long-term fix. The error that the compiler throws
> > is:
> >
> > 	drivers/soc/pxa/ssp.c:155:29: error: cast from pointer to integer of 
> > different size [-Werror=pointer-to-int-cast]
> > 	  155 |                 ssp->type = (int) id->data;
> > 	      |                             ^
> >
> > enum pxa_ssp_type is an integer type, too, and its width could be
> > smaller than 64 bit, too.
> 
> I would just change the cast to (uintptr_t), which is what we
> have elsewhere.
> 
> > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> > index f458469c5ce5..fbe16089e4bb 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -283,7 +283,10 @@ struct of_device_id {
> >  	char	name[32];
> >  	char	type[32];
> >  	char	compatible[128];
> > -	const void *data;
> > +	union {
> > +		const void *data;
> > +		kernel_ulong_t driver_data;
> > +	};
> >  };
> > 
> >  /* VIO */
> >
> > For this driver the change would be nice, as several casts can be
> > dropped.
> 
> I think this is a nice idea in general, but I would keep
> it separate from the bugfix, as we might want to do this on
> a wider scale, or run into problems.

Sure, I didn't intend to put the diff into a commit as is.

Before doing that change it would probably be sensible to check how this
field is used, I guess most drivers use an integer value and not a
pointer there. Also while touching that making the same change with the
same names for the other *_id structs might be nice. Currently we have
(from include/linux/mod_devicetable.h):

	pci_device_id		kernel_ulong_t driver_data
	ieee1394_device_id	kernel_ulong_t driver_data
	usb_device_id		kernel_ulong_t driver_info
	hid_device_id		kernel_ulong_t driver_data
	ccw_device_id		kernel_ulong_t driver_info
	ap_device_id		kernel_ulong_t driver_info
	css_device_id		kernel_ulong_t driver_data
	acpi_device_id		kernel_ulong_t driver_data
	pnp_device_id		kernel_ulong_t driver_data
	pnp_card_device_id	kernel_ulong_t driver_data
	serio_device_id		-
	hda_device_id		unsigned long driver_data
	sdw_device_id		kernel_ulong_t driver_data
	of_device_id		const void *data
	vio_device_id		-
	pcmcia_device_id	kernel_ulong_t driver_info
	input_device_id		kernel_ulong_t driver_info
	eisa_device_id		kernel_ulong_t driver_data
	parisc_device_id	-
	sdio_device_id		kernel_ulong_t driver_data
	ssb_device_id		-
	bcma_device_id		-
	virtio_device_id	-
	hv_vmbus_device_id	kernel_ulong_t driver_data
	rpmsg_device_id		kernel_ulong_t driver_data
	i2c_device_id		kernel_ulong_t driver_data
	pci_epf_device_id	kernel_ulong_t driver_data
	i3c_device_id		const void *data
	spi_device_id		kernel_ulong_t driver_data
	slim_device_id		-
	apr_device_id		kernel_ulong_t driver_data
	spmi_device_id		kernel_ulong_t driver_data
	dmi_system_id		void *driver_data
	platform_device_id	kernel_ulong_t driver_data
	mdio_device_id		-
	zorro_device_id		kernel_ulong_t driver_data
	isapnp_device_id	kernel_ulong_t driver_data
	amba_id			void *data
	mips_cdmm_device_id	-
	x86_cpu_id		kernel_ulong_t driver_data
	ipack_device_id		-
	mei_cl_device_id	kernel_ulong_t driver_info
	rio_device_id		-
	mcb_device_id		kernel_ulong_t driver_data
	ulpi_device_id		kernel_ulong_t driver_data
	fsl_mc_device_id	-
	tb_service_id		kernel_ulong_t driver_data
	typec_device_id		kernel_ulong_t driver_data
	tee_client_device_id	-
	wmi_device_id		const void *context
	mhi_device_id		kernel_ulong_t driver_data
	auxiliary_device_id	kernel_ulong_t driver_data
	ssam_device_id		kernel_ulong_t driver_data
	dfl_device_id		kernel_ulong_t driver_data
	ishtp_device_id		kernel_ulong_t driver_data
	cdx_device_id		-
	vchiq_device_id		-

Note driver_data vs driver_info which is probably not worth to unify.

> In particular, removing tons of casts to (kernel_ulong_t)
> in other subsystems is probably more valuable than removing
> casts to (void *) for of_device_id, since kernel_ulong_t
> is particularly confusing, with a definition that is
> completely unrelated to the similarly named __kernel_ulong_t.

I'll add that to my list of ideas for big projects :-)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature