Re: vmx(4): remove useless code
On Fri, Aug 06, 2021 at 12:06:04PM +0200, Patrick Wildt wrote:
> On Fri, Aug 06, 2021 at 11:05:53AM +0200, Patrick Wildt wrote:
> > Am Thu, Aug 05, 2021 at 02:33:01PM +0200 schrieb Jan Klemkow:
> > > Hi,
> > >
> > > The following diff removes useless code from the driver. As discussed
> > > here [1] and committed there [2], the hypervisor doesn't do anything
> > > with the data structures. We even just set NULL to the pointer since
> > > the initial commit of vmx(4). So, I guess it better to remove all of
> > > these. The variables are bzero'd in vmxnet3_dma_allocmem() anyway.
> > >
> > > OK?
> >
> > My main concern was if the structs are getting zeroed correctly, but
> > they do, so that's fine.
> >
> > That said, it looks like Linux sets the pointer to ~0ULL, not 0. Should
> > we follow Linux' pattern there and do that as well?
> >
>
> Thinking about it a little more, I think we should do that as well. And
> maybe explicitly set driver_data_len to 0 even though it's already zero.
> Basically for readability.
OK?
Index: dev/pci/if_vmx.c
===
RCS file: /cvs/src/sys/dev/pci/if_vmx.c,v
retrieving revision 1.66
diff -u -p -r1.66 if_vmx.c
--- dev/pci/if_vmx.c23 Jul 2021 00:29:14 - 1.66
+++ dev/pci/if_vmx.c6 Aug 2021 12:28:51 -
@@ -157,7 +157,6 @@ struct vmxnet3_softc {
#define WRITE_BAR1(sc, reg, val) \
bus_space_write_4((sc)->sc_iot1, (sc)->sc_ioh1, reg, val)
#define WRITE_CMD(sc, cmd) WRITE_BAR1(sc, VMXNET3_BAR1_CMD, cmd)
-#define vtophys(va) 0 /* XXX ok? */
int vmxnet3_match(struct device *, void *, void *);
void vmxnet3_attach(struct device *, struct device *, void *);
@@ -468,8 +467,8 @@ vmxnet3_dma_init(struct vmxnet3_softc *s
ds->vmxnet3_revision = 1;
ds->upt_version = 1;
ds->upt_features = UPT1_F_CSUM | UPT1_F_VLAN;
- ds->driver_data = vtophys(sc);
- ds->driver_data_len = sizeof(struct vmxnet3_softc);
+ ds->driver_data = ~0ULL;
+ ds->driver_data_len = 0;
ds->queue_shared = qs_pa;
ds->queue_shared_len = qs_len;
ds->mtu = VMXNET3_MAX_MTU;
@@ -546,8 +545,8 @@ vmxnet3_alloc_txring(struct vmxnet3_soft
ts->cmd_ring_len = NTXDESC;
ts->comp_ring = comp_pa;
ts->comp_ring_len = NTXCOMPDESC;
- ts->driver_data = vtophys(tq);
- ts->driver_data_len = sizeof *tq;
+ ts->driver_data = ~0ULL;
+ ts->driver_data_len = 0;
ts->intr_idx = intr;
ts->stopped = 1;
ts->error = 0;
@@ -598,8 +597,8 @@ vmxnet3_alloc_rxring(struct vmxnet3_soft
rs->cmd_ring_len[1] = NRXDESC;
rs->comp_ring = comp_pa;
rs->comp_ring_len = NRXCOMPDESC;
- rs->driver_data = vtophys(rq);
- rs->driver_data_len = sizeof *rq;
+ rs->driver_data = ~0ULL;
+ rs->driver_data_len = 0;
rs->intr_idx = intr;
rs->stopped = 1;
rs->error = 0;
Re: vmx(4): remove useless code
Patrick Wildt wrote: > On Fri, Aug 06, 2021 at 11:05:53AM +0200, Patrick Wildt wrote: > > Am Thu, Aug 05, 2021 at 02:33:01PM +0200 schrieb Jan Klemkow: > > > Hi, > > > > > > The following diff removes useless code from the driver. As discussed > > > here [1] and committed there [2], the hypervisor doesn't do anything > > > with the data structures. We even just set NULL to the pointer since > > > the initial commit of vmx(4). So, I guess it better to remove all of > > > these. The variables are bzero'd in vmxnet3_dma_allocmem() anyway. > > > > > > OK? > > > > My main concern was if the structs are getting zeroed correctly, but > > they do, so that's fine. > > > > That said, it looks like Linux sets the pointer to ~0ULL, not 0. Should > > we follow Linux' pattern there and do that as well? > > > > Thinking about it a little more, I think we should do that as well. And > maybe explicitly set driver_data_len to 0 even though it's already zero. > Basically for readability. The better approach is to zero the whole struct, then fill in non-zero fields... and fill in zero'd fields when doing so idiomaticaly helps future code readers. But if you avoid zero'ing the whole struct, then padding bytes in the struct may be a problem, and depending on the allocator and where the data is later passed to, leak. So full structure zero is the best practice.
Re: vmx(4): remove useless code
On Fri, Aug 06, 2021 at 11:05:53AM +0200, Patrick Wildt wrote:
> Am Thu, Aug 05, 2021 at 02:33:01PM +0200 schrieb Jan Klemkow:
> > Hi,
> >
> > The following diff removes useless code from the driver. As discussed
> > here [1] and committed there [2], the hypervisor doesn't do anything
> > with the data structures. We even just set NULL to the pointer since
> > the initial commit of vmx(4). So, I guess it better to remove all of
> > these. The variables are bzero'd in vmxnet3_dma_allocmem() anyway.
> >
> > OK?
>
> My main concern was if the structs are getting zeroed correctly, but
> they do, so that's fine.
>
> That said, it looks like Linux sets the pointer to ~0ULL, not 0. Should
> we follow Linux' pattern there and do that as well?
>
Thinking about it a little more, I think we should do that as well. And
maybe explicitly set driver_data_len to 0 even though it's already zero.
Basically for readability.
>
> > bye,
> > Jan
> >
> > [1]: https://www.lkml.org/lkml/2021/1/19/1225
> > [2]:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/vmxnet3/vmxnet3_drv.c?id=de1da8bcf40564a2adada2d5d5426e05355f66e8
> >
> > Index: dev/pci/if_vmx.c
> > ===
> > RCS file: /cvs/src/sys/dev/pci/if_vmx.c,v
> > retrieving revision 1.66
> > diff -u -p -r1.66 if_vmx.c
> > --- dev/pci/if_vmx.c23 Jul 2021 00:29:14 - 1.66
> > +++ dev/pci/if_vmx.c5 Aug 2021 11:12:26 -
> > @@ -157,7 +157,6 @@ struct vmxnet3_softc {
> > #define WRITE_BAR1(sc, reg, val) \
> > bus_space_write_4((sc)->sc_iot1, (sc)->sc_ioh1, reg, val)
> > #define WRITE_CMD(sc, cmd) WRITE_BAR1(sc, VMXNET3_BAR1_CMD, cmd)
> > -#define vtophys(va) 0 /* XXX ok? */
> >
> > int vmxnet3_match(struct device *, void *, void *);
> > void vmxnet3_attach(struct device *, struct device *, void *);
> > @@ -468,8 +467,6 @@ vmxnet3_dma_init(struct vmxnet3_softc *s
> > ds->vmxnet3_revision = 1;
> > ds->upt_version = 1;
> > ds->upt_features = UPT1_F_CSUM | UPT1_F_VLAN;
> > - ds->driver_data = vtophys(sc);
> > - ds->driver_data_len = sizeof(struct vmxnet3_softc);
> > ds->queue_shared = qs_pa;
> > ds->queue_shared_len = qs_len;
> > ds->mtu = VMXNET3_MAX_MTU;
> > @@ -546,8 +543,6 @@ vmxnet3_alloc_txring(struct vmxnet3_soft
> > ts->cmd_ring_len = NTXDESC;
> > ts->comp_ring = comp_pa;
> > ts->comp_ring_len = NTXCOMPDESC;
> > - ts->driver_data = vtophys(tq);
> > - ts->driver_data_len = sizeof *tq;
> > ts->intr_idx = intr;
> > ts->stopped = 1;
> > ts->error = 0;
> > @@ -598,8 +593,6 @@ vmxnet3_alloc_rxring(struct vmxnet3_soft
> > rs->cmd_ring_len[1] = NRXDESC;
> > rs->comp_ring = comp_pa;
> > rs->comp_ring_len = NRXCOMPDESC;
> > - rs->driver_data = vtophys(rq);
> > - rs->driver_data_len = sizeof *rq;
> > rs->intr_idx = intr;
> > rs->stopped = 1;
> > rs->error = 0;
> >
>
Re: vmx(4): remove useless code
Am Thu, Aug 05, 2021 at 02:33:01PM +0200 schrieb Jan Klemkow:
> Hi,
>
> The following diff removes useless code from the driver. As discussed
> here [1] and committed there [2], the hypervisor doesn't do anything
> with the data structures. We even just set NULL to the pointer since
> the initial commit of vmx(4). So, I guess it better to remove all of
> these. The variables are bzero'd in vmxnet3_dma_allocmem() anyway.
>
> OK?
My main concern was if the structs are getting zeroed correctly, but
they do, so that's fine.
That said, it looks like Linux sets the pointer to ~0ULL, not 0. Should
we follow Linux' pattern there and do that as well?
Patrick
> bye,
> Jan
>
> [1]: https://www.lkml.org/lkml/2021/1/19/1225
> [2]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/vmxnet3/vmxnet3_drv.c?id=de1da8bcf40564a2adada2d5d5426e05355f66e8
>
> Index: dev/pci/if_vmx.c
> ===
> RCS file: /cvs/src/sys/dev/pci/if_vmx.c,v
> retrieving revision 1.66
> diff -u -p -r1.66 if_vmx.c
> --- dev/pci/if_vmx.c 23 Jul 2021 00:29:14 - 1.66
> +++ dev/pci/if_vmx.c 5 Aug 2021 11:12:26 -
> @@ -157,7 +157,6 @@ struct vmxnet3_softc {
> #define WRITE_BAR1(sc, reg, val) \
> bus_space_write_4((sc)->sc_iot1, (sc)->sc_ioh1, reg, val)
> #define WRITE_CMD(sc, cmd) WRITE_BAR1(sc, VMXNET3_BAR1_CMD, cmd)
> -#define vtophys(va) 0/* XXX ok? */
>
> int vmxnet3_match(struct device *, void *, void *);
> void vmxnet3_attach(struct device *, struct device *, void *);
> @@ -468,8 +467,6 @@ vmxnet3_dma_init(struct vmxnet3_softc *s
> ds->vmxnet3_revision = 1;
> ds->upt_version = 1;
> ds->upt_features = UPT1_F_CSUM | UPT1_F_VLAN;
> - ds->driver_data = vtophys(sc);
> - ds->driver_data_len = sizeof(struct vmxnet3_softc);
> ds->queue_shared = qs_pa;
> ds->queue_shared_len = qs_len;
> ds->mtu = VMXNET3_MAX_MTU;
> @@ -546,8 +543,6 @@ vmxnet3_alloc_txring(struct vmxnet3_soft
> ts->cmd_ring_len = NTXDESC;
> ts->comp_ring = comp_pa;
> ts->comp_ring_len = NTXCOMPDESC;
> - ts->driver_data = vtophys(tq);
> - ts->driver_data_len = sizeof *tq;
> ts->intr_idx = intr;
> ts->stopped = 1;
> ts->error = 0;
> @@ -598,8 +593,6 @@ vmxnet3_alloc_rxring(struct vmxnet3_soft
> rs->cmd_ring_len[1] = NRXDESC;
> rs->comp_ring = comp_pa;
> rs->comp_ring_len = NRXCOMPDESC;
> - rs->driver_data = vtophys(rq);
> - rs->driver_data_len = sizeof *rq;
> rs->intr_idx = intr;
> rs->stopped = 1;
> rs->error = 0;
>
vmx(4): remove useless code
Hi,
The following diff removes useless code from the driver. As discussed
here [1] and committed there [2], the hypervisor doesn't do anything
with the data structures. We even just set NULL to the pointer since
the initial commit of vmx(4). So, I guess it better to remove all of
these. The variables are bzero'd in vmxnet3_dma_allocmem() anyway.
OK?
bye,
Jan
[1]: https://www.lkml.org/lkml/2021/1/19/1225
[2]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/vmxnet3/vmxnet3_drv.c?id=de1da8bcf40564a2adada2d5d5426e05355f66e8
Index: dev/pci/if_vmx.c
===
RCS file: /cvs/src/sys/dev/pci/if_vmx.c,v
retrieving revision 1.66
diff -u -p -r1.66 if_vmx.c
--- dev/pci/if_vmx.c23 Jul 2021 00:29:14 - 1.66
+++ dev/pci/if_vmx.c5 Aug 2021 11:12:26 -
@@ -157,7 +157,6 @@ struct vmxnet3_softc {
#define WRITE_BAR1(sc, reg, val) \
bus_space_write_4((sc)->sc_iot1, (sc)->sc_ioh1, reg, val)
#define WRITE_CMD(sc, cmd) WRITE_BAR1(sc, VMXNET3_BAR1_CMD, cmd)
-#define vtophys(va) 0 /* XXX ok? */
int vmxnet3_match(struct device *, void *, void *);
void vmxnet3_attach(struct device *, struct device *, void *);
@@ -468,8 +467,6 @@ vmxnet3_dma_init(struct vmxnet3_softc *s
ds->vmxnet3_revision = 1;
ds->upt_version = 1;
ds->upt_features = UPT1_F_CSUM | UPT1_F_VLAN;
- ds->driver_data = vtophys(sc);
- ds->driver_data_len = sizeof(struct vmxnet3_softc);
ds->queue_shared = qs_pa;
ds->queue_shared_len = qs_len;
ds->mtu = VMXNET3_MAX_MTU;
@@ -546,8 +543,6 @@ vmxnet3_alloc_txring(struct vmxnet3_soft
ts->cmd_ring_len = NTXDESC;
ts->comp_ring = comp_pa;
ts->comp_ring_len = NTXCOMPDESC;
- ts->driver_data = vtophys(tq);
- ts->driver_data_len = sizeof *tq;
ts->intr_idx = intr;
ts->stopped = 1;
ts->error = 0;
@@ -598,8 +593,6 @@ vmxnet3_alloc_rxring(struct vmxnet3_soft
rs->cmd_ring_len[1] = NRXDESC;
rs->comp_ring = comp_pa;
rs->comp_ring_len = NRXCOMPDESC;
- rs->driver_data = vtophys(rq);
- rs->driver_data_len = sizeof *rq;
rs->intr_idx = intr;
rs->stopped = 1;
rs->error = 0;
