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

[Thomas Weißschuh]

On 2024-04-12 09:45:28+0200, Greg Kroah-Hartman wrote:
> 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> > ---
> > 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.

You can also do

	.driver.field = FOO

which doesn't seem all that crazy and which is already used in many
places.

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

For me it's less about an issue about memory usage but correctness and
developer experience.
It took me some wondering why the attributes didn't show up.

> 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?

The current state which fields are available on which struct foo_driver
seem very ad-hoc. From the ones I looked at:

pci_driver duplicates "name", "groups" and "dev_groups".
(Of which "groups" seems wholly unused)

usb_{device_,}_driver only has "name" and "dev_groups".

platform_driver has none of them.

Instead of adding traces all over the place to me it feels better to
drop the duplication and let the compiler worry about it.

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

Indeed. Do you want me to resend it with

    Fixes: ded13b9cfd59 ("PCI: Add support for dev_groups to struct pci_driver")

?


Thomas