| Message ID | 20210414091832.5132-4-lingshan.zhu@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vDPA/ifcvf: enables Intel C5000X-PL virtio-blk | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
在 2021/4/14 下午5:18, Zhu Lingshan 写道: > get_config_size() should return the size based on the decected > device type. > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> Acked-by: Jason Wang <jasowang@redhat.com> > --- > drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c > index 9b6a38b798fa..b48b9789b69e 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_main.c > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > @@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev) > > static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev) > { > - return sizeof(struct virtio_net_config); > + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); > + size_t size; > + > + if (vf->dev_type == VIRTIO_ID_NET) > + size = sizeof(struct virtio_net_config); > + > + if (vf->dev_type == VIRTIO_ID_BLOCK) > + size = sizeof(struct virtio_blk_config); > + > + return size; > } > > static void ifcvf_vdpa_get_config(struct vdpa_device *vdpa_dev,
On Wed, Apr 14, 2021 at 05:18:32PM +0800, Zhu Lingshan wrote: >get_config_size() should return the size based on the decected >device type. > >Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >--- > drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > >diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c >index 9b6a38b798fa..b48b9789b69e 100644 >--- a/drivers/vdpa/ifcvf/ifcvf_main.c >+++ b/drivers/vdpa/ifcvf/ifcvf_main.c >@@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev) > > static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev) > { >- return sizeof(struct virtio_net_config); >+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >+ size_t size; >+ >+ if (vf->dev_type == VIRTIO_ID_NET) >+ size = sizeof(struct virtio_net_config); >+ >+ if (vf->dev_type == VIRTIO_ID_BLOCK) >+ size = sizeof(struct virtio_blk_config); >+ >+ return size; I'm not familiar with the ifcvf details, but can it happen that the device is not block or net? Should we set `size` to 0 by default to handle this case or are we sure it's one of the two? Maybe we should add a comment or a warning message in this case, to prevent some analysis tool or compiler from worrying that `size` might be uninitialized. I was thinking something like this: switch(vf->dev_type) { case VIRTIO_ID_NET: size = sizeof(struct virtio_net_config); break; case VIRTIO_ID_BLOCK: size = sizeof(struct virtio_blk_config); break; default: /* or WARN(1, "") if dev_warn() not apply */ dev_warn(... , "virtio ID [0x%x] not supported\n") size = 0; } Thanks, Stefano
在 2021/4/15 下午4:12, Stefano Garzarella 写道: > On Wed, Apr 14, 2021 at 05:18:32PM +0800, Zhu Lingshan wrote: >> get_config_size() should return the size based on the decected >> device type. >> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >> --- >> drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c >> b/drivers/vdpa/ifcvf/ifcvf_main.c >> index 9b6a38b798fa..b48b9789b69e 100644 >> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >> @@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct >> vdpa_device *vdpa_dev) >> >> static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev) >> { >> - return sizeof(struct virtio_net_config); >> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >> + size_t size; >> + >> + if (vf->dev_type == VIRTIO_ID_NET) >> + size = sizeof(struct virtio_net_config); >> + >> + if (vf->dev_type == VIRTIO_ID_BLOCK) >> + size = sizeof(struct virtio_blk_config); >> + >> + return size; > > I'm not familiar with the ifcvf details, but can it happen that the > device is not block or net? > > Should we set `size` to 0 by default to handle this case or are we > sure it's one of the two? > > Maybe we should add a comment or a warning message in this case, to > prevent some analysis tool or compiler from worrying that `size` might > be uninitialized. > > I was thinking something like this: > > switch(vf->dev_type) { > case VIRTIO_ID_NET: > size = sizeof(struct virtio_net_config); > break; > case VIRTIO_ID_BLOCK: > size = sizeof(struct virtio_blk_config); > break; > default: > /* or WARN(1, "") if dev_warn() not apply */ > dev_warn(... , "virtio ID [0x%x] not supported\n") > size = 0; > > } > Yes, I agree. Thanks > Thanks, > Stefano >
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c index 9b6a38b798fa..b48b9789b69e 100644 --- a/drivers/vdpa/ifcvf/ifcvf_main.c +++ b/drivers/vdpa/ifcvf/ifcvf_main.c @@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev) static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev) { - return sizeof(struct virtio_net_config); + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); + size_t size; + + if (vf->dev_type == VIRTIO_ID_NET) + size = sizeof(struct virtio_net_config); + + if (vf->dev_type == VIRTIO_ID_BLOCK) + size = sizeof(struct virtio_blk_config); + + return size; } static void ifcvf_vdpa_get_config(struct vdpa_device *vdpa_dev,
get_config_size() should return the size based on the decected device type. Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> --- drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)