> Subject: RE: [Patch v4 03/12] net: mana: Handle vport sharing between devices
>
> > From: longli@xxxxxxxxxxxxxxxxx <longli@xxxxxxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxx>
