Re: [PATCH 4.18 101/135] hv_netvsc: pair VF based on serial number

From: Greg Kroah-Hartman
Date: Wed Oct 17 2018 - 09:17:23 EST

On Wed, Oct 17, 2018 at 01:02:17PM +0000, Haiyang Zhang wrote:
>
>
> > -----Original Message-----
> > From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Sent: Wednesday, October 17, 2018 3:46 AM
> > To: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx; Stephen Hemminger
> > <sthemmin@xxxxxxxxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>; Sasha
> > Levin <Alexander.Levin@xxxxxxxxxxxxx>
> > Subject: Re: [PATCH 4.18 101/135] hv_netvsc: pair VF based on serial number
> >
> > On Tue, Oct 16, 2018 at 07:13:12PM +0000, Haiyang Zhang wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: linux-kernel-owner@xxxxxxxxxxxxxxx <linux-kernel-
> > > > owner@xxxxxxxxxxxxxxx> On Behalf Of Greg Kroah-Hartman
> > > > Sent: Tuesday, October 16, 2018 1:06 PM
> > > > To: linux-kernel@xxxxxxxxxxxxxxx
> > > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>;
> > > > stable@xxxxxxxxxxxxxxx; Stephen Hemminger <sthemmin@xxxxxxxxxxxxx>;
> > > > David S. Miller <davem@xxxxxxxxxxxxx>; Sasha Levin
> > > > <Alexander.Levin@xxxxxxxxxxxxx>
> > > > Subject: [PATCH 4.18 101/135] hv_netvsc: pair VF based on serial
> > > > number
> > > >
> > > > 4.18-stable review patch. If anyone has any objections, please let me know.
> > > >
> > > > ------------------
> > > >
> > > > From: Stephen Hemminger <stephen@xxxxxxxxxxxxxxxxxx>
> > > >
> > > > [ Upstream commit 00d7ddba1143623b31bc2c15d18216e2da031b14 ]
> > > >
> > > > Matching network device based on MAC address is problematic since a
> > > > non VF network device can be creted with a duplicate MAC address
> > > > causing confusion and problems. The VMBus API does provide a serial
> > > > number that is a better matching method.
> > > >
> > > > Signed-off-by: Stephen Hemminger <sthemmin@xxxxxxxxxxxxx>
> > > > Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
> > > > Signed-off-by: Sasha Levin <alexander.levin@xxxxxxxxxxxxx>
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > > drivers/net/hyperv/netvsc.c | 3 ++
> > > > drivers/net/hyperv/netvsc_drv.c | 58 ++++++++++++++++++++++------------
> > -----
> > > > -
> > > > 2 files changed, 36 insertions(+), 25 deletions(-)
> > > >
> > > > --- a/drivers/net/hyperv/netvsc.c
> > > > +++ b/drivers/net/hyperv/netvsc.c
> > > > @@ -1203,6 +1203,9 @@ static void netvsc_send_vf(struct net_de
> > > >
> > > > net_device_ctx->vf_alloc = nvmsg->msg.v4_msg.vf_assoc.allocated;
> > > > net_device_ctx->vf_serial = nvmsg->msg.v4_msg.vf_assoc.serial;
> > > > + netdev_info(ndev, "VF slot %u %s\n",
> > > > + net_device_ctx->vf_serial,
> > > > + net_device_ctx->vf_alloc ? "added" : "removed");
> > > > }
> > > >
> > > > static void netvsc_receive_inband(struct net_device *ndev,
> > > > --- a/drivers/net/hyperv/netvsc_drv.c
> > > > +++ b/drivers/net/hyperv/netvsc_drv.c
> > > > @@ -1794,20 +1794,6 @@ out_unlock:
> > > > rtnl_unlock();
> > > > }
> > > >
> > > > -static struct net_device *get_netvsc_bymac(const u8 *mac) -{
> > > > - struct net_device_context *ndev_ctx;
> > > > -
> > > > - list_for_each_entry(ndev_ctx, &netvsc_dev_list, list) {
> > > > - struct net_device *dev = hv_get_drvdata(ndev_ctx-
> > > > >device_ctx);
> > > > -
> > > > - if (ether_addr_equal(mac, dev->perm_addr))
> > > > - return dev;
> > > > - }
> > > > -
> > > > - return NULL;
> > > > -}
> > > > -
> > > > static struct net_device *get_netvsc_byref(struct net_device *vf_netdev) {
> > > > struct net_device_context *net_device_ctx; @@ -1936,26 +1922,48
> > @@
> > > > static void netvsc_vf_setup(struct work_
> > > > rtnl_unlock();
> > > > }
> > > >
> > > > +/* Find netvsc by VMBus serial number.
> > > > + * The PCI hyperv controller records the serial number as the slot.
> > > > + */
> > > > +static struct net_device *get_netvsc_byslot(const struct net_device
> > > > +*vf_netdev) {
> > > > + struct device *parent = vf_netdev->dev.parent;
> > > > + struct net_device_context *ndev_ctx;
> > > > + struct pci_dev *pdev;
> > > > +
> > > > + if (!parent || !dev_is_pci(parent))
> > > > + return NULL; /* not a PCI device */
> > > > +
> > > > + pdev = to_pci_dev(parent);
> > > > + if (!pdev->slot) {
> > > > + netdev_notice(vf_netdev, "no PCI slot information\n");
> > > > + return NULL;
> > > > + }
> > > > +
> > > > + list_for_each_entry(ndev_ctx, &netvsc_dev_list, list) {
> > > > + if (!ndev_ctx->vf_alloc)
> > > > + continue;
> > > > +
> > > > + if (ndev_ctx->vf_serial == pdev->slot->number)
> > > > + return hv_get_drvdata(ndev_ctx->device_ctx);
> > >
> > > Greg,
> > >
> > > I had a fix to this patch. If not already, Could you include my following fix
> > together?
> > > hv_netvsc: fix vf serial matching with pci slot info
> > > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.k
> > > ernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fdavem%2Fnet-
> > next.git%2F
> > >
> > commit%2F%3Fid%3D005479556197f80139771960dda0dfdcd2d2aad5&amp;da
> > ta=02%
> > >
> > 7C01%7Chaiyangz%40microsoft.com%7C6ce194a47449435fe82a08d63404a94e
> > %7C7
> > >
> > 2f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636753591959679786&amp
> > ;sdata
> > >
> > =ReWYMZW%2FYtoFSbFSxny9VGw6V8v9J4sGanfJjbVd34A%3D&amp;reserved=
> > 0
> >
> > I can not take a patch that is not "upstream" yet, sorry.
> >
> > Also, this really feels like a "new feature" to me. Can someone explain what
> > bug this is fixing?
>
> Patch #A: hv_netvsc: pair VF based on serial number
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=00d7ddba1143623b31bc2c15d18216e2da031b14
>
> Patch #B: hv_netvsc: fix vf serial matching with pci slot info
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=005479556197f80139771960dda0dfdcd2d2aad5
>
> The patch #A is a feature that use VF serial number for matching. But #A has a bug causing mismatch.
>
> Patch #B, upstream now (see the link to Linux-next), is a fix to #A.

linux-next is not "upstream", it needs to be in Linus's tree, you know
this :(

> So, if #A is ported to 4.18, we need to port #B together. Otherwise, not porting both the #A and #B is also fine.

You didn't answer my original question here, what bug is this fixing?

I'll go drop this from the tree for now anyway...

thanks,

greg k-h