Re: [PATCH v4 6/9] agp/amd-k7: Don't rely on address of pci_device_id

Thank you for your contribution! Sashiko AI review found 2 potential issue(=
s) to consider:
- [Medium] Storing kernel string pointers in driver_data prevents adding dy=
namic IDs via sysfs (new_id).
- [Low] Binding the driver via driver_override passes a NULL pointer to a %=
s format specifier.
--

--- Patch [6]: [PATCH v4 6/9] agp/amd-k7: Don't rely on address of pci_devi=
ce_id ---
commit 6e944a8c8b19b7d71cd09452e813daa32931ee55
Author: Gary Guo <[email protected]>

agp/amd-k7: Don't rely on address of pci_device_id
   =20
Address of pci_device_id cannot be relied on due to presence of dynamic ID
and driver_override. Use driver_data instead.

> diff --git a/drivers/char/agp/amd-k7-agp.c b/drivers/char/agp/amd-k7-agp.c
> index 898ff30ffd46b..4d201e71c5177 100644
> --- a/drivers/char/agp/amd-k7-agp.c
> +++ b/drivers/char/agp/amd-k7-agp.c

[ ... ]

> @@ -387,37 +387,17 @@ static const struct agp_bridge_driver amd_irongate_=
driver =3D {
>  	.agp_type_to_mask_type  =3D agp_generic_type_to_mask_type,
>  };
> =20
> -static struct agp_device_ids amd_agp_device_ids[] =3D
> -{
> -	{
> -		.device_id	=3D PCI_DEVICE_ID_AMD_FE_GATE_7006,
> -		.chipset_name	=3D "Irongate",
> -	},
> -	{
> -		.device_id	=3D PCI_DEVICE_ID_AMD_FE_GATE_700E,
> -		.chipset_name	=3D "761",
> -	},
> -	{
> -		.device_id	=3D PCI_DEVICE_ID_AMD_FE_GATE_700C,
> -		.chipset_name	=3D "760MP",
> -	},
> -	{ }, /* dummy final entry, always present */
> -};
> -
>  static int agp_amdk7_probe(struct pci_dev *pdev,
>  			   const struct pci_device_id *ent)
>  {
>  	struct agp_bridge_data *bridge;
>  	u8 cap_ptr;
> -	int j;
> =20
>  	cap_ptr =3D pci_find_capability(pdev, PCI_CAP_ID_AGP);
>  	if (!cap_ptr)
>  		return -ENODEV;
> =20
> -	j =3D ent - agp_amdk7_pci_table;
> -	dev_info(&pdev->dev, "AMD %s chipset\n",
> -		 amd_agp_device_ids[j].chipset_name);
> +	dev_info(&pdev->dev, "AMD %s chipset\n", (const char *)ent->driver_data=
);

[Severity: Low]
Does this pass a NULL pointer to a %s format specifier when the driver is b=
ound
via driver_override?

When a device is bound using the sysfs driver_override interface, the PCI c=
ore
passes a dummy pci_device_id_any structure where driver_data is 0 (NULL). T=
his
would result in an ungraceful log message like "AMD (null) chipset".

Should we add a fallback check for NULL and print a string like "Unknown"
instead?

> =20
>  	bridge =3D agp_alloc_bridge();
>  	if (!bridge)

[ ... ]

> @@ -492,7 +472,6 @@ static int agp_amdk7_resume(struct device *dev)
>  	return amd_irongate_driver.configure();
>  }
> =20
> -/* must be the same order as name table above */
>  static const struct pci_device_id agp_amdk7_pci_table[] =3D {
>  	{
>  	.class		=3D (PCI_CLASS_BRIDGE_HOST << 8),
> @@ -501,6 +480,7 @@ static const struct pci_device_id agp_amdk7_pci_table=
[] =3D {
>  	.device		=3D PCI_DEVICE_ID_AMD_FE_GATE_7006,
>  	.subvendor	=3D PCI_ANY_ID,
>  	.subdevice	=3D PCI_ANY_ID,
> +	.driver_data	=3D (kernel_ulong_t)"Irongate",

[Severity: Medium]
Will storing kernel memory addresses (string literals) in driver_data preve=
nt
users from adding dynamic IDs via sysfs?

The PCI core's new_id_store validates the provided driver_data (which defau=
lts
to 0) against the driver_data in the driver's static id_table. Since this p=
atch
assigns string pointers to driver_data, users cannot supply a matching value
via the new_id sysfs interface, causing new_id_store to always reject the i=
nput
with -EINVAL.

Could the driver store an enum or integer index in driver_data and use it to
look up the string array instead?

>  	},
>  	{
>  	.class		=3D (PCI_CLASS_BRIDGE_HOST << 8),

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260723-pci_id_fix=
[email protected]?part=3D6