> > On Wed, Mar 27, 2024 at 09:37:43AM +0800, Xuan Zhuo wrote:
> > > On Tue, 26 Mar 2024 08:19:08 -0700, Breno Leitao <lei...@debian.org>
> > > wrote:
> > > > Do not set virtnet_info->rss_indir_table_size if RSS is not available
> > > > for the device.
> > > >
> > > > Currently, rss_indir_table_size is set if either has_rss or
> > > > has_rss_hash_report is available, but, it should only be set if has_rss
> > > > is set.
> > > >
> > > > On the virtnet_set_rxfh(), return an invalid command if the request has
> > > > indirection table set, but virtnet does not support RSS.
> > > >
> > > > Suggested-by: Heng Qi <hen...@linux.alibaba.com>
> > > > Signed-off-by: Breno Leitao <lei...@debian.org>
> > > > ---
> > > > drivers/net/virtio_net.c | 9 +++++++--
> > > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index c22d1118a133..c640fdf28fc5 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -3813,6 +3813,9 @@ static int virtnet_set_rxfh(struct net_device
> > > > *dev,
> > > > rxfh->hfunc != ETH_RSS_HASH_TOP)
> > > > return -EOPNOTSUPP;
> > > >
> > > > + if (rxfh->indir && !vi->has_rss)
> > > > + return -EINVAL;
> > > > +
> > > > if (rxfh->indir) {
> > >
> > > Put !vi->has_rss here?
> >
> > I am not sure I understand the suggestion. Where do you suggest we have
> > !vi->has_rss?
> >
> > If we got this far, we either have:
> >
> > a) rxfh->indir set and vi->has_rss is also set
> > b) rxfh->indir not set. (vi->has_rss could be set or not).
>
>
> This function does two tasks.
> 1. update indir table
> 2. update rss key
>
> #1 only for has_rss
> #2 for has_rss or has_rss_hash_report
>
>
> So I would code:
>
> bool update = false
>
> if (rxfh->indir) {
> if (!vi->has_rss)
> return -EINVAL;
>
> for (i = 0; i < vi->rss_indir_table_size; ++i)
> vi->ctrl->rss.indirection_table[i] = rxfh->indir[i];
>
> update = true
> }
>
> if (rxfh->key) {
> if (!vi->has_rss && !vi->has_rss_hash_report)
> return -EINVAL;
>
> memcpy(vi->ctrl->rss.key, rxfh->key, vi->rss_key_size);
> update = true
> }
>
>
> if (update)
> virtnet_commit_rss_command(vi);
>
> Thanks.
This is a bit different from the previous proposal, but, I like this one
approach better. It makes the code easier to read.
How does the full patch looks like?
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index c22d1118a133..180f342f1898 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3807,20 +3807,35 @@ static int virtnet_set_rxfh(struct net_device *dev,
struct netlink_ext_ack *extack)
{
struct virtnet_info *vi = netdev_priv(dev);
+ bool update = false;
int i;
+ if (!vi->has_rss && !vi->has_rss_hash_report)
+ return -EOPNOTSUPP;
+
if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE &&
rxfh->hfunc != ETH_RSS_HASH_TOP)
return -EOPNOTSUPP;
if (rxfh->indir) {
+ if (!vi->has_rss)
+ return -EINVAL;
+
for (i = 0; i < vi->rss_indir_table_size; ++i)
vi->ctrl->rss.indirection_table[i] = rxfh->indir[i];
+ update = true;
}
- if (rxfh->key)
+
+ if (rxfh->key) {
+ if (!vi->has_rss && !vi->has_rss_hash_report)
+ return -EINVAL;
+
memcpy(vi->ctrl->rss.key, rxfh->key, vi->rss_key_size);
+ update = true;
+ }
- virtnet_commit_rss_command(vi);
+ if (update)
+ virtnet_commit_rss_command(vi);
return 0;
}
@@ -4729,13 +4744,15 @@ static int virtnet_probe(struct virtio_device *vdev)
if (virtio_has_feature(vdev, VIRTIO_NET_F_HASH_REPORT))
vi->has_rss_hash_report = true;
- if (virtio_has_feature(vdev, VIRTIO_NET_F_RSS))
+ if (virtio_has_feature(vdev, VIRTIO_NET_F_RSS)) {
vi->has_rss = true;
- if (vi->has_rss || vi->has_rss_hash_report) {
vi->rss_indir_table_size =
virtio_cread16(vdev, offsetof(struct virtio_net_config,
rss_max_indirection_table_length));
+ }
+
+ if (vi->has_rss || vi->has_rss_hash_report) {
vi->rss_key_size =
virtio_cread8(vdev, offsetof(struct virtio_net_config,
rss_max_key_size));
