Stefano Garzarella
2021-Apr-15 13:48 UTC
[PATCH V2 3/3] vDPA/ifcvf: get_config_size should return dev specific config size
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 at 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
Zhu Lingshan
2021-Apr-16 02:42 UTC
[PATCH V2 3/3] vDPA/ifcvf: get_config_size should return dev specific config size
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 at 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 at lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization