Received-SPF: pass (google.com: domain of linux-kernel+bounces-142172-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Fri, 12 Apr 2024 09:45:28 +0200
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Thomas =?iso-8859-1?Q?Wei=DFschuh?= <linux@weissschuh.net>
Cc: Arnd Bergmann <arnd@arndb.de>, Mihai Carabas <mihai.carabas@oracle.com>,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] misc/pvpanic-pci: register attributes via pci_driver
Message-ID: <2024041228-maximum-aware-f078@gregkh>
References: <20240411-pvpanic-pci-dev-groups-v1-1-db8cb69f1b09@weissschuh.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240411-pvpanic-pci-dev-groups-v1-1-db8cb69f1b09@weissschuh.net>

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