| Message ID | 20210415095336.4792-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 |
On Thu, Apr 15, 2021 at 05:53:36PM +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 | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > >diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c >index cea1313b1a3f..6844c49fe1de 100644 >--- a/drivers/vdpa/ifcvf/ifcvf_main.c >+++ b/drivers/vdpa/ifcvf/ifcvf_main.c >@@ -347,7 +347,23 @@ 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_adapter *adapter = vdpa_to_adapter(vdpa_dev); >+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >+ struct pci_dev *pdev = adapter->pdev; >+ size_t size; >+ >+ if (vf->dev_type == VIRTIO_ID_NET) >+ size = sizeof(struct virtio_net_config); >+ >+ else if (vf->dev_type == VIRTIO_ID_BLOCK) >+ size = sizeof(struct virtio_blk_config); >+ >+ else { >+ size = 0; >+ IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", vf->dev_type); >+ } I slightly prefer the switch, but I don't have a strong opinion. However, if we want to use if/else, we should follow `Documentation/process/coding-style.rst` line 166: Note that the closing brace is empty on a line of its own, **except** in the cases where it is followed by a continuation of the same statement, ie a ``while`` in a do-statement or an ``else`` in an if-statement, like also `scripts/checkpatch.pl --strict` complains: CHECK: braces {} should be used on all arms of this statement #209: FILE: drivers/vdpa/ifcvf/ifcvf_main.c:355: + if (vf->dev_type == VIRTIO_ID_NET) [...] + else if (vf->dev_type == VIRTIO_ID_BLOCK) [...] + else { [...] CHECK: Unbalanced braces around else statement #215: FILE: drivers/vdpa/ifcvf/ifcvf_main.c:361: + else { Thanks, Stefano
On 4/15/2021 9:48 PM, Stefano Garzarella wrote: > On Thu, Apr 15, 2021 at 05:53:36PM +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 | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c >> b/drivers/vdpa/ifcvf/ifcvf_main.c >> index cea1313b1a3f..6844c49fe1de 100644 >> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >> @@ -347,7 +347,23 @@ 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_adapter *adapter = vdpa_to_adapter(vdpa_dev); >> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >> + struct pci_dev *pdev = adapter->pdev; >> + size_t size; >> + >> + if (vf->dev_type == VIRTIO_ID_NET) >> + size = sizeof(struct virtio_net_config); >> + >> + else if (vf->dev_type == VIRTIO_ID_BLOCK) >> + size = sizeof(struct virtio_blk_config); >> + >> + else { >> + size = 0; >> + IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", vf->dev_type); >> + } > > I slightly prefer the switch, but I don't have a strong opinion. > > However, if we want to use if/else, we should follow > `Documentation/process/coding-style.rst` line 166: > Note that the closing brace is empty on a line of its own, > **except** in > the cases where it is followed by a continuation of the same > statement, > ie a ``while`` in a do-statement or an ``else`` in an if-statement, > like > > also `scripts/checkpatch.pl --strict` complains: > > CHECK: braces {} should be used on all arms of this statement > #209: FILE: drivers/vdpa/ifcvf/ifcvf_main.c:355: > + if (vf->dev_type == VIRTIO_ID_NET) > [...] > + else if (vf->dev_type == VIRTIO_ID_BLOCK) > [...] > + else { > [...] > > CHECK: Unbalanced braces around else statement > #215: FILE: drivers/vdpa/ifcvf/ifcvf_main.c:361: > + else { Thanks Stefano, the reason is we only have one line code after if, so looks like {} is unnecessary, I agree switch can clear up code style confusions. I will add this in v3. Thanks! > > Thanks, > Stefano > > _______________________________________________ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c index cea1313b1a3f..6844c49fe1de 100644 --- a/drivers/vdpa/ifcvf/ifcvf_main.c +++ b/drivers/vdpa/ifcvf/ifcvf_main.c @@ -347,7 +347,23 @@ 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_adapter *adapter = vdpa_to_adapter(vdpa_dev); + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); + struct pci_dev *pdev = adapter->pdev; + size_t size; + + if (vf->dev_type == VIRTIO_ID_NET) + size = sizeof(struct virtio_net_config); + + else if (vf->dev_type == VIRTIO_ID_BLOCK) + size = sizeof(struct virtio_blk_config); + + else { + size = 0; + IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", vf->dev_type); + } + + 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 | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)