Re: [PATCH] misc/pvpanic-pci: register attributes via pci_driver

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Apr 11, 2024 at 11:33:51PM +0200, Thomas Weißschuh wrote:
> In __pci_register_driver(), the pci core overwrites the dev_groups field of
> the embedded struct device_driver with the dev_groups from the outer
> struct pci_driver unconditionally.
> 
> Set dev_groups in the pci_driver to make sure it is used.
> 
> This was broken since the introduction of pvpanic-pci.
> 
> Fixes: db3a4f0abefd ("misc/pvpanic: add PCI driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> Greg,
> 
> does it make sense to duplicate fields between struct pci_driver and
> struct device_driver?
> The fields "name", "groups" and "dev_groups" are duplicated.
> 
> pci_driver::dev_groups was introduced in
> commit ded13b9cfd59 ("PCI: Add support for dev_groups to struct pci_driver")
> because "this helps converting PCI drivers sysfs attributes to static"
> 
> I don't understand the reasoning. The embedded device_driver shares the
> same storage lifetime and the fields have the exact same type.

It's "simpler" to have the fields be in the pci_driver structure as then
you don't need to do the crazy:
	.driver = {
		.field = FOO,
	},

type of declaration just for simplicity.

And as the number overall of these structures is very very small,
duplication on the driver level is not really an issue.

Duplication on a device level is another story, there should not be any
duplication at all if possible there, as that is where it really
matters.

> ---
>  drivers/misc/pvpanic/pvpanic-pci.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/misc/pvpanic/pvpanic-pci.c b/drivers/misc/pvpanic/pvpanic-pci.c
> index 9ad20e82785b..b21598a18f6d 100644
> --- a/drivers/misc/pvpanic/pvpanic-pci.c
> +++ b/drivers/misc/pvpanic/pvpanic-pci.c
> @@ -44,8 +44,6 @@ static struct pci_driver pvpanic_pci_driver = {
>  	.name =         "pvpanic-pci",
>  	.id_table =     pvpanic_pci_id_tbl,
>  	.probe =        pvpanic_pci_probe,
> -	.driver = {
> -		.dev_groups = pvpanic_dev_groups,
> -	},
> +	.dev_groups =   pvpanic_dev_groups,

Maybe we should throw a trace in the pci core if we find that dev_groups
is set to something before we override it to catch this type of mistake
in the future?

Although, given that this never worked in the first place, it seems odd
that the original developer never noticed it, so perhaps that's not
really an issue here.

Oh wait, it originally did, but the pci change caused it to break,
nevermind, it is relevent, thanks.

thanks,

greg k-h