netdev - RE: [Patch v4 03/12] net: mana: Handle vport sharing between devices

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <PH7PR21MB3263F08C111C5D06C99CC32ACE869@PH7PR21MB3263.namprd21.prod.outlook.com>
Date:   Tue, 12 Jul 2022 18:48:09 +0000
From:   Long Li <longli@...rosoft.com>
To:     Dexuan Cui <decui@...rosoft.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Wei Liu <wei.liu@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Leon Romanovsky <leon@...nel.org>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "shiraz.saleem@...el.com" <shiraz.saleem@...el.com>,
        Ajay Sharma <sharmaajay@...rosoft.com>
CC:     "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>
Subject: RE: [Patch v4 03/12] net: mana: Handle vport sharing between devices

> Subject: RE: [Patch v4 03/12] net: mana: Handle vport sharing between devices
> 
> > From: longli@...uxonhyperv.com <longli@...uxonhyperv.com>
> > Sent: Wednesday, June 15, 2022 7:07 PM
> > +void mana_uncfg_vport(struct mana_port_context *apc) {
> > +	mutex_lock(&apc->vport_mutex);
> > +	apc->vport_use_count--;
> > +	WARN_ON(apc->vport_use_count < 0);
> > +	mutex_unlock(&apc->vport_mutex);
> > +}
> > +EXPORT_SYMBOL_GPL(mana_uncfg_vport);
> > +
> > +int mana_cfg_vport(struct mana_port_context *apc, u32 protection_dom_id,
> > +		   u32 doorbell_pg_id)
> >  {
> >  	struct mana_config_vport_resp resp = {};
> >  	struct mana_config_vport_req req = {};
> >  	int err;
> >
> > +	/* Ethernet driver and IB driver can't take the port at the same time */
> > +	mutex_lock(&apc->vport_mutex);
> > +	if (apc->vport_use_count > 0) {
> > +		mutex_unlock(&apc->vport_mutex);
> > +		return -ENODEV;
> Maybe -EBUSY is better?

I agree with you, EBUSY is a better value. Will change this in v5.

> 
> > @@ -563,9 +581,19 @@ static int mana_cfg_vport(struct
> > mana_port_context *apc, u32 protection_dom_id,
> >
> >  	apc->tx_shortform_allowed = resp.short_form_allowed;
> >  	apc->tx_vp_offset = resp.tx_vport_offset;
> > +
> > +	netdev_info(apc->ndev, "Configured vPort %llu PD %u DB %u\n",
> > +		    apc->port_handle, protection_dom_id, doorbell_pg_id);
> Should this be netdev_dbg()?
> The log buffer can be flooded if there are many vPorts per VF PCI device and
> there are a lot of VFs.

The reason netdev_info () is used is that this message is important for troubleshooting initial setup issues with Ethernet driver. We rely on user to get this configured right to share the same hardware port between Ethernet and RDMA driver. As far as I know, there is no easy way for a driver to "take over" an exclusive hardware resource from another driver. 

If it is acceptable that we have one such message for each opened Ethernet port on the system, I suggest we keep it this way.

> 
> >  out:
> > +	if (err) {
> > +		mutex_lock(&apc->vport_mutex);
> > +		apc->vport_use_count--;
> > +		mutex_unlock(&apc->vport_mutex);
> > +	}
> 
> Change this to the blelow?
>     if (err)
>         mana_uncfg_vport(apc);
> 
> > @@ -626,6 +654,9 @@ static int mana_cfg_vport_steering(struct
> > mana_port_context *apc,
> >  			   resp.hdr.status);
> >  		err = -EPROTO;
> >  	}
> > +
> > +	netdev_info(ndev, "Configured steering vPort %llu entries %u\n",
> > +		    apc->port_handle, num_entries);
> 
> netdev_dbg()?
> 
> In general, the patch looks good to me.
> Reviewed-by: Dexuan Cui <decui@...rosoft.com>