netdev - RE: [PATCH 07/22] RDMA/irdma: Register an auxiliary driver and implement private channel OPs

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: <99895f7c10a2473c84a105f46c7ef498@intel.com>
Date:   Tue, 26 Jan 2021 00:42:16 +0000
From:   "Saleem, Shiraz" <shiraz.saleem@...el.com>
To:     Jason Gunthorpe <jgg@...dia.com>
CC:     "dledford@...hat.com" <dledford@...hat.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Ertman, David M" <david.m.ertman@...el.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "Ismail, Mustafa" <mustafa.ismail@...el.com>
Subject: RE: [PATCH 07/22] RDMA/irdma: Register an auxiliary driver and
 implement private channel OPs

> Subject: Re: [PATCH 07/22] RDMA/irdma: Register an auxiliary driver and
> implement private channel OPs
> 
> On Fri, Jan 22, 2021 at 05:48:12PM -0600, Shiraz Saleem wrote:
> > +/**
> > + * irdma_init_dev - GEN_2 device init
> > + * @aux_dev: auxiliary device
> > + *
> > + * Create device resources, set up queues, pble and hmc objects.
> > + * Return 0 if successful, otherwise return error  */ int
> > +irdma_init_dev(struct auxiliary_device *aux_dev) {
> > +	struct iidc_auxiliary_object *vo = container_of(aux_dev,
> > +							struct
> iidc_auxiliary_object,
> > +							adev);
> > +	struct iidc_peer_obj *peer_info = vo->peer_obj;
> > +	struct irdma_handler *hdl;
> > +	struct irdma_pci_f *rf;
> > +	struct irdma_sc_dev *dev;
> > +	struct irdma_priv_peer_info *priv_peer_info;
> > +	int err;
> > +
> > +	hdl = irdma_find_handler(peer_info->pdev);
> > +	if (hdl)
> > +		return -EBUSY;
> > +
> > +	hdl = kzalloc(sizeof(*hdl), GFP_KERNEL);
> > +	if (!hdl)
> > +		return -ENOMEM;
> > +
> > +	rf = &hdl->rf;
> > +	priv_peer_info = &rf->priv_peer_info;
> > +	rf->aux_dev = aux_dev;
> > +	rf->hdl = hdl;
> > +	dev = &rf->sc_dev;
> > +	dev->back_dev = rf;
> > +	rf->gen_ops.init_hw = icrdma_init_hw;
> > +	rf->gen_ops.request_reset = icrdma_request_reset;
> > +	rf->gen_ops.register_qset = irdma_lan_register_qset;
> > +	rf->gen_ops.unregister_qset = irdma_lan_unregister_qset;
> > +	priv_peer_info->peer_info = peer_info;
> > +	rf->rdma_ver = IRDMA_GEN_2;
> > +	irdma_set_config_params(rf);
> > +	dev->pci_rev = peer_info->pdev->revision;
> > +	rf->default_vsi.vsi_idx = peer_info->pf_vsi_num;
> > +	/* save information from peer_info to priv_peer_info*/
> > +	priv_peer_info->fn_num = PCI_FUNC(peer_info->pdev->devfn);
> > +	rf->hw.hw_addr = peer_info->hw_addr;
> > +	rf->pcidev = peer_info->pdev;
> > +	rf->netdev = peer_info->netdev;
> > +	priv_peer_info->ftype = peer_info->ftype;
> > +	priv_peer_info->msix_count = peer_info->msix_count;
> > +	priv_peer_info->msix_entries = peer_info->msix_entries;
> > +	irdma_add_handler(hdl);
> > +	if (irdma_ctrl_init_hw(rf)) {
> > +		err = -EIO;
> > +		goto err_ctrl_init;
> > +	}
> > +	peer_info->peer_ops = &irdma_peer_ops;
> > +	peer_info->peer_drv = &irdma_peer_drv;
> > +	err = peer_info->ops->peer_register(peer_info);
> > +	if (err)
> > +		goto err_peer_reg;
> 
> No to this, I don't want to see aux bus layered on top of another management
> framework in new drivers. When this driver uses aux bus get rid of the old i40iw
> stuff. I already said this in one of the older postings of this driver.
> 
> auxbus probe() for a RDMA driver should call ib_alloc_device() near its start and
> ib_register_device() near the end its end.
> 
> drvdata for the aux device should point to the driver struct containing the
> ib_device.
> 
> Just like any ordinary PCI based rdma driver, look at how something like
> pvrdma_pci_probe() is structued.
> 

I think this essentially means doing away with .open/.close piece. Or are you saying that is ok?
Yes we had a discussion in the past and I thought we concluded. But maybe I misunderstood.

https://lore.kernel.org/linux-rdma/9DD61F30A802C4429A01CA4200E302A7DCD4FD03@fmsmsx124.amr.corp.intel.com/

The concern is around losing the resources held for rdma VFs by rdma PF due to underlying config changes which require a de-reg/re-registration of the ibdevice.
Today such config changes are handled with the netdev PCI driver using the .close() private callback into rdma driver which unregister ibdevice in PF while allowing
the RDMA VF to survive.

Shiraz