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

From: Jason Gunthorpe <jgg@nvidia.com>
To: Shiraz Saleem <shiraz.saleem@intel.com>
Cc: dledford@redhat.com, kuba@kernel.org, davem@davemloft.net,
	linux-rdma@vger.kernel.org, netdev@vger.kernel.org,
	david.m.ertman@intel.com, anthony.l.nguyen@intel.com
Subject: Re: [PATCH v4 resend 01/23] iidc: Introduce iidc.h
Date: Wed, 7 Apr 2021 15:05:29 -0300	[thread overview]
Message-ID: <20210407180529.GA547588@nvidia.com> (raw)
In-Reply-To: <20210407001502.1890-2-shiraz.saleem@intel.com>

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