netdev - Re: [PATCH v4 resend 01/23] iidc: Introduce iidc.h

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210407180529.GA547588@nvidia.com>
Date:   Wed, 7 Apr 2021 15:05:29 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Shiraz Saleem <shiraz.saleem@...el.com>
Cc:     dledford@...hat.com, kuba@...nel.org, davem@...emloft.net,
        linux-rdma@...r.kernel.org, netdev@...r.kernel.org,
        david.m.ertman@...el.com, anthony.l.nguyen@...el.com
Subject: Re: [PATCH v4 resend 01/23] iidc: Introduce iidc.h

On Tue, Apr 06, 2021 at 07:14:40PM -0500, Shiraz Saleem wrote:
> +/* Structure representing auxiliary driver tailored information about the core
> + * PCI dev, each auxiliary driver using the IIDC interface will have an
> + * instance of this struct dedicated to it.
> + */
> +struct iidc_core_dev_info {
> +	struct pci_dev *pdev; /* PCI device of corresponding to main function */
> +	struct auxiliary_device *adev;
> +	/* KVA / Linear address corresponding to BAR0 of underlying
> +	 * pci_device.
> +	 */
> +	u8 __iomem *hw_addr;
> +	int cdev_info_id;
> +
> +	u8 ftype;	/* PF(false) or VF (true) */

Where is ftype initialized?

> +	u16 vport_id;
> +	enum iidc_rdma_protocol rdma_protocol;

This duplicates the aux device name, not really sure why it is
needed. One user just uses it to make the string, the rest is
entangled with the devlink and doesn't need to be stored here.

> +	struct iidc_qos_params qos_info;
> +	struct net_device *netdev;
> +
> +	struct msix_entry *msix_entries;
> +	u16 msix_count; /* How many vectors are reserved for this device */
> +
> +	/* Following struct contains function pointers to be initialized
> +	 * by core PCI driver and called by auxiliary driver
> +	 */
> +	const struct iidc_core_ops *ops;
> +};

I spent a while trying to understand this struct and why it exists
and..

> +
> +struct iidc_auxiliary_dev {
> +	struct auxiliary_device adev;
> +	struct iidc_core_dev_info *cdev_info;

This cdev_info should just be a 'struct ice_pf *' and the "struct
iidc_core_dev_info" should be deleted entirely. You'll notice this
ends up looking nearly exactly like mlx5 does after this.

You can see it clearly based on how this gets initialized:

		cdev_info->pdev = pf->pdev;
		cdev_info->hw_addr = (u8 __iomem *)pf->hw.hw_addr;

                struct ice_vsi *vsi = ice_get_main_vsi(pf);
		cdev_info->vport_id = vsi->vsi_num;
		cdev_info->netdev = vsi->netdev;
		cdev_info->msix_count = pf->num_rdma_msix;
		cdev_info->msix_entries = &pf->msix_entries[pf->rdma_base_vector];

		ice_setup_dcb_qos_info(pf, cdev_info->qos_info);

Since the main place this cdev_info appears is in the ops API between
the two modules, it looks to me like this is being designed in this
obfuscated way to try and create a stable ABI between two modules.

Remove all the stable module ABI hackery before you resend it.

Jason