On Thu, Aug 1, 2024 at 2:43 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
>
> On Thu, Aug 01, 2024 at 02:13:49PM +0800, Jason Wang wrote:
> > On Thu, Aug 1, 2024 at 2:06 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
> > >
> > > On Wed, Jul 31, 2024 at 10:59:47AM +0800, Jason Wang wrote:
> > > > This patch synchronize operstate with admin state per RFC2863.
> > > >
> > > > This is done by trying to toggle the carrier upon open/close and
> > > > synchronize with the config change work. This allows propagate status
> > > > correctly to stacked devices like:
> > > >
> > > > ip link add link enp0s3 macvlan0 type macvlan
> > > > ip link set link enp0s3 down
> > > > ip link show
> > > >
> > > > Before this patch:
> > > >
> > > > 3: enp0s3: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000
> > > > link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > > > ......
> > > > 5: macvlan0@enp0s3: <BROADCAST,MULTICAST,UP,LOWER_UP,M-DOWN> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
> > > > link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> > > >
> > > > After this patch:
> > > >
> > > > 3: enp0s3: <BROADCAST,MULTICAST> mtu 1500 qdisc pfifo_fast state DOWN mode DEFAULT group default qlen 1000
> > > > link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> > > > ...
> > > > 5: macvlan0@enp0s3: <NO-CARRIER,BROADCAST,MULTICAST,UP,M-DOWN> mtu 1500 qdisc noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
> > > > link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
> > > >
> > > > Cc: Venkat Venkatsubra <venkat.x.venkatsubra@xxxxxxxxxx>
> > > > Cc: Gia-Khanh Nguyen <gia-khanh.nguyen@xxxxxxxxxx>
> > > > Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
> > > > ---
> > > > drivers/net/virtio_net.c | 84 ++++++++++++++++++++++++++--------------
> > > > 1 file changed, 54 insertions(+), 30 deletions(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 0383a3e136d6..0cb93261eba1 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -2878,6 +2878,7 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
> > > > return err;
> > > > }
> > > >
> > > > +
> > > > static void virtnet_cancel_dim(struct virtnet_info *vi, struct dim *dim)
> > > > {
> > > > if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL))
> > > > @@ -2885,6 +2886,25 @@ static void virtnet_cancel_dim(struct virtnet_info *vi, struct dim *dim)
> > > > net_dim_work_cancel(dim);
> > > > }
> > > >
> > > > +static void virtnet_update_settings(struct virtnet_info *vi)
> > > > +{
> > > > + u32 speed;
> > > > + u8 duplex;
> > > > +
> > > > + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX))
> > > > + return;
> > > > +
> > > > + virtio_cread_le(vi->vdev, struct virtio_net_config, speed, &speed);
> > > > +
> > > > + if (ethtool_validate_speed(speed))
> > > > + vi->speed = speed;
> > > > +
> > > > + virtio_cread_le(vi->vdev, struct virtio_net_config, duplex, &duplex);
> > > > +
> > > > + if (ethtool_validate_duplex(duplex))
> > > > + vi->duplex = duplex;
> > > > +}
> > > > +
> > > > static int virtnet_open(struct net_device *dev)
> > > > {
> > > > struct virtnet_info *vi = netdev_priv(dev);
> > > > @@ -2903,6 +2923,16 @@ static int virtnet_open(struct net_device *dev)
> > > > goto err_enable_qp;
> > > > }
> > > >
> > > > + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
> > > > + if (vi->status & VIRTIO_NET_S_LINK_UP)
> > > > + netif_carrier_on(vi->dev);
> > > > + virtio_config_driver_enable(vi->vdev);
> > > > + } else {
> > > > + vi->status = VIRTIO_NET_S_LINK_UP;
> > > > + netif_carrier_on(dev);
> > > > + virtnet_update_settings(vi);
> > > > + }
> > > > +
> > > > return 0;
> > > >
> > > > err_enable_qp:
> > > > @@ -3381,12 +3411,18 @@ static int virtnet_close(struct net_device *dev)
> > > > disable_delayed_refill(vi);
> > > > /* Make sure refill_work doesn't re-enable napi! */
> > > > cancel_delayed_work_sync(&vi->refill);
> > > > + /* Make sure config notification doesn't schedule config work */
> > > > + virtio_config_driver_disable(vi->vdev);
> > > > + /* Make sure status updating is cancelled */
> > > > + cancel_work_sync(&vi->config_work);
> > > >
> > > > for (i = 0; i < vi->max_queue_pairs; i++) {
> > > > virtnet_disable_queue_pair(vi, i);
> > > > virtnet_cancel_dim(vi, &vi->rq[i].dim);
> > > > }
> > > >
> > > > + netif_carrier_off(dev);
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > > @@ -5085,25 +5121,6 @@ static void virtnet_init_settings(struct net_device *dev)
> > > > vi->duplex = DUPLEX_UNKNOWN;
> > > > }
> > > >
> > > > -static void virtnet_update_settings(struct virtnet_info *vi)
> > > > -{
> > > > - u32 speed;
> > > > - u8 duplex;
> > > > -
> > > > - if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX))
> > > > - return;
> > > > -
> > > > - virtio_cread_le(vi->vdev, struct virtio_net_config, speed, &speed);
> > > > -
> > > > - if (ethtool_validate_speed(speed))
> > > > - vi->speed = speed;
> > > > -
> > > > - virtio_cread_le(vi->vdev, struct virtio_net_config, duplex, &duplex);
> > > > -
> > > > - if (ethtool_validate_duplex(duplex))
> > > > - vi->duplex = duplex;
> > > > -}
> > > > -
> > > > static u32 virtnet_get_rxfh_key_size(struct net_device *dev)
> > > > {
> > > > return ((struct virtnet_info *)netdev_priv(dev))->rss_key_size;
> > > > @@ -6514,6 +6531,11 @@ static int virtnet_probe(struct virtio_device *vdev)
> > > > goto free_failover;
> > > > }
> > > >
> > > > + /* Forbid config change notification until ndo_open. */
> > > > + virtio_config_driver_disable(vi->vdev);
> > > > + /* Make sure status updating work is done */
> > >
> > > Wait a second, how can anything run here, this is probe,
> > > config change callbacks are never invoked at all.
> >
> > For buggy devices.
>
> buggy or not, virtio core will not invoke callbacks until
> after probe and scan.
I think you're right, but we still need the
virtio_config_driver_disable(). Otherwise the config change callback
might race with ndo_open().
>
> > >
> > > > + cancel_work_sync(&vi->config_work);
> > > > +
> > >
> > >
> > > this is pointless, too.
> > >
> > > > virtio_device_ready(vdev);
> > > >
> > > > virtnet_set_queues(vi, vi->curr_queue_pairs);
> > > > @@ -6563,6 +6585,19 @@ static int virtnet_probe(struct virtio_device *vdev)
> > > > vi->device_stats_cap = le64_to_cpu(v);
> > > > }
> > > >
> > > > + /* Assume link up if device can't report link status,
> > > > + otherwise get link status from config. */
> > > > + netif_carrier_off(dev);
> > > > + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
> > > > + /* This is safe as config notification change has been
> > > > + disabled. */
> > >
> > > What "this"? pls explain what this does: get config data from
> > > device.
> > >
> > > Actually not because it was disabled. probe can poke at
> > > config with impunity no change callbacks trigger during probe.
> >
> > Only if we have a good device.
>
> not if you read the core code.
Right after the device is ready the config notification might be
triggered by the device, but the callback will be blocked by the core.
Thanks
>
> > >
> > > > + virtnet_config_changed_work(&vi->config_work);
> > >
> > >
> > >
> >
> > Thanks
> >
> >
> > >
> > > > + } else {
> > > > + vi->status = VIRTIO_NET_S_LINK_UP;
> > > > + virtnet_update_settings(vi);
> > > > + netif_carrier_on(dev);
> > > > + }
> > > > +
> > > > rtnl_unlock();
> > > >
> > > > err = virtnet_cpu_notif_add(vi);
> > > > @@ -6571,17 +6606,6 @@ static int virtnet_probe(struct virtio_device *vdev)
> > > > goto free_unregister_netdev;
> > > > }
> > > >
> > > > - /* Assume link up if device can't report link status,
> > > > - otherwise get link status from config. */
> > > > - netif_carrier_off(dev);
> > > > - if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
> > > > - schedule_work(&vi->config_work);
> > > > - } else {
> > > > - vi->status = VIRTIO_NET_S_LINK_UP;
> > > > - virtnet_update_settings(vi);
> > > > - netif_carrier_on(dev);
> > > > - }
> > > > -
> > > > for (i = 0; i < ARRAY_SIZE(guest_offloads); i++)
> > > > if (virtio_has_feature(vi->vdev, guest_offloads[i]))
> > > > set_bit(guest_offloads[i], &vi->guest_offloads);
> > > > --
> > > > 2.31.1
> > >
>
