Paolo Abeni
2023-Jan-31 09:01 UTC
[PATCH v3 2/2] virtio_net: notify MAC address change on device initialization
On Fri, 2023-01-27 at 21:45 +0100, Laurent Vivier wrote:> In virtnet_probe(), if the device doesn't provide a MAC address the > driver assigns a random one. > As we modify the MAC address we need to notify the device to allow it > to update all the related information. > > The problem can be seen with vDPA and mlx5_vdpa driver as it doesn't > assign a MAC address by default. The virtio_net device uses a random > MAC address (we can see it with "ip link"), but we can't ping a net > namespace from another one using the virtio-vdpa device because the > new MAC address has not been provided to the hardware: > RX packets are dropped since they don't go through the receive filters, > TX packets go through unaffected. > > Signed-off-by: Laurent Vivier <lvivier at redhat.com> > --- > drivers/net/virtio_net.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 7d700f8e545a..704a05f1c279 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -3806,6 +3806,8 @@ static int virtnet_probe(struct virtio_device *vdev) > eth_hw_addr_set(dev, addr); > } else { > eth_hw_addr_random(dev); > + dev_info(&vdev->dev, "Assigned random MAC address %pM\n", > + dev->dev_addr); > } > > /* Set up our device-specific information */ > @@ -3933,6 +3935,24 @@ static int virtnet_probe(struct virtio_device *vdev) > > virtio_device_ready(vdev); > > + /* a random MAC address has been assigned, notify the device. > + * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not there > + * because many devices work fine without getting MAC explicitly > + */ > + if (!virtio_has_feature(vdev, VIRTIO_NET_F_MAC) && > + virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) { > + struct scatterlist sg; > + > + sg_init_one(&sg, dev->dev_addr, dev->addr_len); > + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC, > + VIRTIO_NET_CTRL_MAC_ADDR_SET, &sg)) { > + pr_debug("virtio_net: setting MAC address failed\n"); > + rtnl_unlock(); > + err = -EINVAL; > + goto free_unregister_netdev;Since the above is still dealing with device initialization, would it make sense moving such init step before registering the netdevice? Cheers, Paolo
Laurent Vivier
2023-Jan-31 09:32 UTC
[PATCH v3 2/2] virtio_net: notify MAC address change on device initialization
On 1/31/23 10:01, Paolo Abeni wrote:> On Fri, 2023-01-27 at 21:45 +0100, Laurent Vivier wrote: >> In virtnet_probe(), if the device doesn't provide a MAC address the >> driver assigns a random one. >> As we modify the MAC address we need to notify the device to allow it >> to update all the related information. >> >> The problem can be seen with vDPA and mlx5_vdpa driver as it doesn't >> assign a MAC address by default. The virtio_net device uses a random >> MAC address (we can see it with "ip link"), but we can't ping a net >> namespace from another one using the virtio-vdpa device because the >> new MAC address has not been provided to the hardware: >> RX packets are dropped since they don't go through the receive filters, >> TX packets go through unaffected. >> >> Signed-off-by: Laurent Vivier <lvivier at redhat.com> >> --- >> drivers/net/virtio_net.c | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >> index 7d700f8e545a..704a05f1c279 100644 >> --- a/drivers/net/virtio_net.c >> +++ b/drivers/net/virtio_net.c >> @@ -3806,6 +3806,8 @@ static int virtnet_probe(struct virtio_device *vdev) >> eth_hw_addr_set(dev, addr); >> } else { >> eth_hw_addr_random(dev); >> + dev_info(&vdev->dev, "Assigned random MAC address %pM\n", >> + dev->dev_addr); >> } >> >> /* Set up our device-specific information */ >> @@ -3933,6 +3935,24 @@ static int virtnet_probe(struct virtio_device *vdev) >> >> virtio_device_ready(vdev); >> >> + /* a random MAC address has been assigned, notify the device. >> + * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not there >> + * because many devices work fine without getting MAC explicitly >> + */ >> + if (!virtio_has_feature(vdev, VIRTIO_NET_F_MAC) && >> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) { >> + struct scatterlist sg; >> + >> + sg_init_one(&sg, dev->dev_addr, dev->addr_len); >> + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC, >> + VIRTIO_NET_CTRL_MAC_ADDR_SET, &sg)) { >> + pr_debug("virtio_net: setting MAC address failed\n"); >> + rtnl_unlock(); >> + err = -EINVAL; >> + goto free_unregister_netdev; > > Since the above is still dealing with device initialization, would it > make sense moving such init step before registering the netdevice?It depends if we can send the command using the control command queue or not. I don't think we can use a vq before virtio_device_ready(). Thanks, Laurent
Michael S. Tsirkin
2023-Jan-31 13:43 UTC
[PATCH v3 2/2] virtio_net: notify MAC address change on device initialization
On Tue, Jan 31, 2023 at 10:01:53AM +0100, Paolo Abeni wrote:> On Fri, 2023-01-27 at 21:45 +0100, Laurent Vivier wrote: > > In virtnet_probe(), if the device doesn't provide a MAC address the > > driver assigns a random one. > > As we modify the MAC address we need to notify the device to allow it > > to update all the related information. > > > > The problem can be seen with vDPA and mlx5_vdpa driver as it doesn't > > assign a MAC address by default. The virtio_net device uses a random > > MAC address (we can see it with "ip link"), but we can't ping a net > > namespace from another one using the virtio-vdpa device because the > > new MAC address has not been provided to the hardware: > > RX packets are dropped since they don't go through the receive filters, > > TX packets go through unaffected. > > > > Signed-off-by: Laurent Vivier <lvivier at redhat.com> > > --- > > drivers/net/virtio_net.c | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 7d700f8e545a..704a05f1c279 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -3806,6 +3806,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > eth_hw_addr_set(dev, addr); > > } else { > > eth_hw_addr_random(dev); > > + dev_info(&vdev->dev, "Assigned random MAC address %pM\n", > > + dev->dev_addr); > > } > > > > /* Set up our device-specific information */ > > @@ -3933,6 +3935,24 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > virtio_device_ready(vdev); > > > > + /* a random MAC address has been assigned, notify the device. > > + * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not there > > + * because many devices work fine without getting MAC explicitly > > + */ > > + if (!virtio_has_feature(vdev, VIRTIO_NET_F_MAC) && > > + virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)) { > > + struct scatterlist sg; > > + > > + sg_init_one(&sg, dev->dev_addr, dev->addr_len); > > + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MAC, > > + VIRTIO_NET_CTRL_MAC_ADDR_SET, &sg)) { > > + pr_debug("virtio_net: setting MAC address failed\n"); > > + rtnl_unlock(); > > + err = -EINVAL; > > + goto free_unregister_netdev; > > Since the above is still dealing with device initialization, would it > make sense moving such init step before registering the netdevice? > > Cheers, > > PaoloWe can't really, device has to be ready otherwise we can't send commands to it. -- MST