LinuxLists.cc - [PATCH] staging: vc04_services: vchiq

2024-06-13 19:42:05

Subject: [PATCH] staging: vc04_services: vchiq_arm: Fix initialisation check

The vchiq_state used to be obtained through an accessor
which would validate that the VCHIQ had been initialised
correctly with the remote.

In commit 42a2f6664e18 ("staging: vc04_services: Move global g_state to
vchiq_state") the global state was moved to the vchiq_mgnt structures
stored as a vchiq instance specific context. This conversion removed the
helpers and instead replaced users of this helper with the assumption
that the state is always available and the remote connected.

Fix this broken assumption by re-introducing the logic that was lost
during the conversion.

Fixes: 42a2f6664e18 ("staging: vc04_services: Move global g_state to vchiq_state")
Signed-off-by: Kieran Bingham <[email protected]>
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
.../staging/vc04_services/interface/vchiq_arm/vchiq_core.h | 5 +++++
.../staging/vc04_services/interface/vchiq_arm/vchiq_dev.c | 7 ++++++-
3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 54467be8c371..67d853f5f2a0 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -804,7 +804,7 @@ int vchiq_initialise(struct vchiq_state *state, struct vchiq_instance **instance
* block forever.
*/
for (i = 0; i < VCHIQ_INIT_RETRIES; i++) {
- if (state)
+ if (vchiq_remote_initialised(state))
break;
usleep_range(500, 600);
}
@@ -1299,7 +1299,7 @@ void vchiq_dump_platform_instances(struct vchiq_state *state, struct seq_file *f
{
int i;

- if (!state)
+ if (!vchiq_remote_initialised(state))
return;

/*
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index 8af209e34fb2..382ec08f6a14 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -413,6 +413,11 @@ struct vchiq_state {
struct opaque_platform_state *platform_state;
};

+static inline bool vchiq_remote_initialised(const struct vchiq_state *state)
+{
+ return state->remote && state->remote->initialised;
+}
+
struct bulk_waiter {
struct vchiq_bulk *bulk;
struct completion event;
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
index 3c63347d2d08..8c4830df1070 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -1170,6 +1170,11 @@ static int vchiq_open(struct inode *inode, struct file *file)

dev_dbg(state->dev, "arm: vchiq open\n");

+ if (!vchiq_remote_initialised(state)) {
+ dev_err(state->dev, "arm: vchiq has no connection to VideoCore\n");
+ return -ENOTCONN;
+ }
+
instance = kzalloc(sizeof(*instance), GFP_KERNEL);
if (!instance)
return -ENOMEM;
@@ -1200,7 +1205,7 @@ static int vchiq_release(struct inode *inode, struct file *file)

dev_dbg(state->dev, "arm: instance=%p\n", instance);

- if (!state) {
+ if (!vchiq_remote_initialised(state)) {
ret = -EPERM;
goto out;
}
--
2.34.1

2024-06-13 20:08:01

by Dan Carpenter

[permalink] [raw]

Subject: Re: [PATCH] staging: vc04_services: vchiq_arm: Fix initialisation check

On Thu, Jun 13, 2024 at 08:41:45PM +0100, Kieran Bingham wrote:
> ---
> .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
> .../staging/vc04_services/interface/vchiq_arm/vchiq_core.h | 5 +++++
> .../staging/vc04_services/interface/vchiq_arm/vchiq_dev.c | 7 ++++++-
> 3 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 54467be8c371..67d853f5f2a0 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -804,7 +804,7 @@ int vchiq_initialise(struct vchiq_state *state, struct vchiq_instance **instance
> * block forever.
> */
> for (i = 0; i < VCHIQ_INIT_RETRIES; i++) {
> - if (state)
> + if (vchiq_remote_initialised(state))
> break;
> usleep_range(500, 600);
> }

:/ In the original code, this would either break on the first iteration
or fail. The diff looked like this:

for (i = 0; i < VCHIQ_INIT_RETRIES; i++) {
- state = vchiq_get_state();
if (state)
break;

I feel bad for not spotting this. A static checker which looked at
diffs could have made this work, but all of our tools look at a momement
in time instead of looking at the change over time.

regards,
dan carpenter

2024-06-13 20:08:20

by Stefan Wahren

[permalink] [raw]

Subject: Re: [PATCH] staging: vc04_services: vchiq_arm: Fix initialisation check

Hi Kieran,

Am 13.06.24 um 21:41 schrieb Kieran Bingham:
> The vchiq_state used to be obtained through an accessor
> which would validate that the VCHIQ had been initialised
> correctly with the remote.
>
> In commit 42a2f6664e18 ("staging: vc04_services: Move global g_state to
> vchiq_state") the global state was moved to the vchiq_mgnt structures
> stored as a vchiq instance specific context. This conversion removed the
> helpers and instead replaced users of this helper with the assumption
> that the state is always available and the remote connected.
>
> Fix this broken assumption by re-introducing the logic that was lost
> during the conversion.
thank you for sending this patch. Maybe it's worth to mention that this
patch also drop some unnecessary NULL checks of state.
>
> Fixes: 42a2f6664e18 ("staging: vc04_services: Move global g_state to vchiq_state")
> Signed-off-by: Kieran Bingham <[email protected]>
> ---
> .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
> .../staging/vc04_services/interface/vchiq_arm/vchiq_core.h | 5 +++++
> .../staging/vc04_services/interface/vchiq_arm/vchiq_dev.c | 7 ++++++-
> 3 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 54467be8c371..67d853f5f2a0 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -804,7 +804,7 @@ int vchiq_initialise(struct vchiq_state *state, struct vchiq_instance **instance
> * block forever.
> */
> for (i = 0; i < VCHIQ_INIT_RETRIES; i++) {
> - if (state)
> + if (vchiq_remote_initialised(state))
> break;
> usleep_range(500, 600);
> }
> @@ -1299,7 +1299,7 @@ void vchiq_dump_platform_instances(struct vchiq_state *state, struct seq_file *f
> {
> int i;
>
> - if (!state)
> + if (!vchiq_remote_initialised(state))
> return;
>
> /*
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> index 8af209e34fb2..382ec08f6a14 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> @@ -413,6 +413,11 @@ struct vchiq_state {
> struct opaque_platform_state *platform_state;
> };
>
> +static inline bool vchiq_remote_initialised(const struct vchiq_state *state)
> +{
> + return state->remote && state->remote->initialised;
> +}
> +
> struct bulk_waiter {
> struct vchiq_bulk *bulk;
> struct completion event;
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> index 3c63347d2d08..8c4830df1070 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> @@ -1170,6 +1170,11 @@ static int vchiq_open(struct inode *inode, struct file *file)
>
> dev_dbg(state->dev, "arm: vchiq open\n");
>
> + if (!vchiq_remote_initialised(state)) {
> + dev_err(state->dev, "arm: vchiq has no connection to VideoCore\n");
Can you please downgrade the log level to dev_dbg, because vchiq_open is
called from userspace, so we can prevent log spamming?
> + return -ENOTCONN;
> + }
> +
> instance = kzalloc(sizeof(*instance), GFP_KERNEL);
> if (!instance)
> return -ENOMEM;
> @@ -1200,7 +1205,7 @@ static int vchiq_release(struct inode *inode, struct file *file)
>
> dev_dbg(state->dev, "arm: instance=%p\n", instance);
>
> - if (!state) {
> + if (!vchiq_remote_initialised(state)) {
> ret = -EPERM;
> goto out;
> }

2024-06-13 20:09:59

by Kieran Bingham

[permalink] [raw]

Subject: [PATCH] staging: vc04_services: vchiq_arm: Fix initialisation check

2024-06-14 06:50:36

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH] staging: vc04_services: vchiq_arm: Fix initialisation check

On Thu, Jun 13, 2024 at 08:41:46PM +0100, Kieran Bingham wrote:
> The vchiq_state used to be obtained through an accessor
> which would validate that the VCHIQ had been initialised
> correctly with the remote.
>
> In commit 42a2f6664e18 ("staging: vc04_services: Move global g_state to
> vchiq_state") the global state was moved to the vchiq_mgnt structures
> stored as a vchiq instance specific context. This conversion removed the
> helpers and instead replaced users of this helper with the assumption
> that the state is always available and the remote connected.
>
> Fix this broken assumption by re-introducing the logic that was lost
> during the conversion.
>
> Fixes: 42a2f6664e18 ("staging: vc04_services: Move global g_state to vchiq_state")
> Signed-off-by: Kieran Bingham <[email protected]>
> ---
> .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
> .../staging/vc04_services/interface/vchiq_arm/vchiq_core.h | 5 +++++
> .../staging/vc04_services/interface/vchiq_arm/vchiq_dev.c | 7 ++++++-
> 3 files changed, 13 insertions(+), 3 deletions(-)

You sent 2 different patches here, both with the same subject, which one
is correct?

Please send a v2 so that I know what to apply, I've dropped this one
from my queue now, thanks.

greg k-h

2024-06-14 10:52:43

by Kieran Bingham

[permalink] [raw]

Subject: Re: [PATCH] staging: vc04_services: vchiq_arm: Fix initialisation check

Quoting Greg Kroah-Hartman (2024-06-14 07:50:08)
> On Thu, Jun 13, 2024 at 08:41:46PM +0100, Kieran Bingham wrote:
> > The vchiq_state used to be obtained through an accessor
> > which would validate that the VCHIQ had been initialised
> > correctly with the remote.
> >
> > In commit 42a2f6664e18 ("staging: vc04_services: Move global g_state to
> > vchiq_state") the global state was moved to the vchiq_mgnt structures
> > stored as a vchiq instance specific context. This conversion removed the
> > helpers and instead replaced users of this helper with the assumption
> > that the state is always available and the remote connected.
> >
> > Fix this broken assumption by re-introducing the logic that was lost
> > during the conversion.
> >
> > Fixes: 42a2f6664e18 ("staging: vc04_services: Move global g_state to vchiq_state")
> > Signed-off-by: Kieran Bingham <[email protected]>
> > ---
> > .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
> > .../staging/vc04_services/interface/vchiq_arm/vchiq_core.h | 5 +++++
> > .../staging/vc04_services/interface/vchiq_arm/vchiq_dev.c | 7 ++++++-
> > 3 files changed, 13 insertions(+), 3 deletions(-)
>
> You sent 2 different patches here, both with the same subject, which one
> is correct?

What in the git ... ? (history formatted to new lines..)

kbingham@Monstersaurus:~/iob/libcamera/raspberrypi/sources/linux$ history | grep send-email
2427 git send-email \
./0001-staging-vc04_services-vchiq_arm-Fix-initialisation-c.patch \
--cc-cmd ./scripts/get_maintainer.pl \
./0001-staging-vc04_services-vchiq_arm-Fix-initialisation-c.patch \
--to "Umang Jain <[email protected]>" \
--to "Florian Fainelli <[email protected]>" \
--to "[email protected]"

I .... uh ... somehow specified the patch twice...

So both mails are the identical patch. Would you like me to send a third
identical one as a v2? or pick either of these?

in fact - never mind - v2 coming.

--
Kieran

>
> Please send a v2 so that I know what to apply, I've dropped this one
> from my queue now, thanks.
>
> greg k-h

2024-06-14 10:55:11

by Kieran Bingham

[permalink] [raw]

Subject: Re: [PATCH] staging: vc04_services: vchiq_arm: Fix initialisation check

Quoting Dan Carpenter (2024-06-13 20:58:40)
> On Thu, Jun 13, 2024 at 08:41:45PM +0100, Kieran Bingham wrote:
> > ---
> > .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
> > .../staging/vc04_services/interface/vchiq_arm/vchiq_core.h | 5 +++++
> > .../staging/vc04_services/interface/vchiq_arm/vchiq_dev.c | 7 ++++++-
> > 3 files changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > index 54467be8c371..67d853f5f2a0 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -804,7 +804,7 @@ int vchiq_initialise(struct vchiq_state *state, struct vchiq_instance **instance
> > * block forever.
> > */
> > for (i = 0; i < VCHIQ_INIT_RETRIES; i++) {
> > - if (state)
> > + if (vchiq_remote_initialised(state))
> > break;
> > usleep_range(500, 600);
> > }
>
> :/ In the original code, this would either break on the first iteration
> or fail. The diff looked like this:
>
> for (i = 0; i < VCHIQ_INIT_RETRIES; i++) {
> - state = vchiq_get_state();
> if (state)
> break;
>
> I feel bad for not spotting this. A static checker which looked at

Definitely don't feel bad - I'm pretty sure I looked through the patch
on it's way through too and missed it then!

I only spotted this once I went deeper and was doing more rework, so it
suddenly stood out with more context.

Unfortunately - it's one of the pains of limited context in diffs in a
mail client I think.

--
Kieran

> diffs could have made this work, but all of our tools look at a momement
> in time instead of looking at the change over time.
>
> regards,
> dan carpenter
>

2024-06-14 11:36:22

by Kieran Bingham

[permalink] [raw]

Subject: Re: [PATCH] staging: vc04_services: vchiq_arm: Fix initialisation check

Hi Stefan,

Sorry, indeed I completely missed this mail.

Quoting Stefan Wahren (2024-06-13 21:01:42)
> Hi Kieran,
>
> Am 13.06.24 um 21:41 schrieb Kieran Bingham:
> > The vchiq_state used to be obtained through an accessor
> > which would validate that the VCHIQ had been initialised
> > correctly with the remote.
> >
> > In commit 42a2f6664e18 ("staging: vc04_services: Move global g_state to
> > vchiq_state") the global state was moved to the vchiq_mgnt structures
> > stored as a vchiq instance specific context. This conversion removed the
> > helpers and instead replaced users of this helper with the assumption
> > that the state is always available and the remote connected.
> >
> > Fix this broken assumption by re-introducing the logic that was lost
> > during the conversion.
>
> thank you for sending this patch. Maybe it's worth to mention that this
> patch also drop some unnecessary NULL checks of state.

I don't understand this comment. Nothing is dropped is it?

The newly added vchiq_remote_initialised() is itself a null-check too!

> > Fixes: 42a2f6664e18 ("staging: vc04_services: Move global g_state to vchiq_state")
> > Signed-off-by: Kieran Bingham <[email protected]>
> > ---
> > .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
> > .../staging/vc04_services/interface/vchiq_arm/vchiq_core.h | 5 +++++
> > .../staging/vc04_services/interface/vchiq_arm/vchiq_dev.c | 7 ++++++-
> > 3 files changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > index 54467be8c371..67d853f5f2a0 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -804,7 +804,7 @@ int vchiq_initialise(struct vchiq_state *state, struct vchiq_instance **instance
> > * block forever.
> > */
> > for (i = 0; i < VCHIQ_INIT_RETRIES; i++) {
> > - if (state)
> > + if (vchiq_remote_initialised(state))
> > break;
> > usleep_range(500, 600);
> > }
> > @@ -1299,7 +1299,7 @@ void vchiq_dump_platform_instances(struct vchiq_state *state, struct seq_file *f
> > {
> > int i;
> >
> > - if (!state)
> > + if (!vchiq_remote_initialised(state))
> > return;
> >
> > /*
> > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> > index 8af209e34fb2..382ec08f6a14 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> > @@ -413,6 +413,11 @@ struct vchiq_state {
> > struct opaque_platform_state *platform_state;
> > };
> >
> > +static inline bool vchiq_remote_initialised(const struct vchiq_state *state)
> > +{
> > + return state->remote && state->remote->initialised;
> > +}
> > +
> > struct bulk_waiter {
> > struct vchiq_bulk *bulk;
> > struct completion event;
> > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> > index 3c63347d2d08..8c4830df1070 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
> > @@ -1170,6 +1170,11 @@ static int vchiq_open(struct inode *inode, struct file *file)
> >
> > dev_dbg(state->dev, "arm: vchiq open\n");
> >
> > + if (!vchiq_remote_initialised(state)) {
> > + dev_err(state->dev, "arm: vchiq has no connection to VideoCore\n");
> Can you please downgrade the log level to dev_dbg, because vchiq_open is
> called from userspace, so we can prevent log spamming?

Sure.

> > + return -ENOTCONN;
> > + }
> > +
> > instance = kzalloc(sizeof(*instance), GFP_KERNEL);
> > if (!instance)
> > return -ENOMEM;
> > @@ -1200,7 +1205,7 @@ static int vchiq_release(struct inode *inode, struct file *file)
> >
> > dev_dbg(state->dev, "arm: instance=%p\n", instance);
> >
> > - if (!state) {
> > + if (!vchiq_remote_initialised(state)) {
> > ret = -EPERM;
> > goto out;
> > }
>

2024-06-14 11:53:14

by Stefan Wahren

[permalink] [raw]

Subject: Re: [PATCH] staging: vc04_services: vchiq_arm: Fix initialisation check

Hi Kieran,

Am 14.06.24 um 13:36 schrieb Kieran Bingham:
> Hi Stefan,
>
> Sorry, indeed I completely missed this mail.
>
> Quoting Stefan Wahren (2024-06-13 21:01:42)
>> Hi Kieran,
>>
>> Am 13.06.24 um 21:41 schrieb Kieran Bingham:
>>> The vchiq_state used to be obtained through an accessor
>>> which would validate that the VCHIQ had been initialised
>>> correctly with the remote.
>>>
>>> In commit 42a2f6664e18 ("staging: vc04_services: Move global g_state to
>>> vchiq_state") the global state was moved to the vchiq_mgnt structures
>>> stored as a vchiq instance specific context. This conversion removed the
>>> helpers and instead replaced users of this helper with the assumption
>>> that the state is always available and the remote connected.
>>>
>>> Fix this broken assumption by re-introducing the logic that was lost
>>> during the conversion.
>> thank you for sending this patch. Maybe it's worth to mention that this
>> patch also drop some unnecessary NULL checks of state.
> I don't understand this comment. Nothing is dropped is it?
>
> The newly added vchiq_remote_initialised() is itself a null-check too!
the vchiq_remote_initialised() only checks the member remote, but not
state itself. From my point of view the null-check for state is
unnecessary, because most of the code already assumed that state is not
null like e.g. in vchiq_open().

2024-06-14 12:00:36

by Kieran Bingham

[permalink] [raw]

Subject: Re: [PATCH] staging: vc04_services: vchiq_arm: Fix initialisation check

Quoting Stefan Wahren (2024-06-14 12:52:53)
> Hi Kieran,
>
> Am 14.06.24 um 13:36 schrieb Kieran Bingham:
> > Hi Stefan,
> >
> > Sorry, indeed I completely missed this mail.
> >
> > Quoting Stefan Wahren (2024-06-13 21:01:42)
> >> Hi Kieran,
> >>
> >> Am 13.06.24 um 21:41 schrieb Kieran Bingham:
> >>> The vchiq_state used to be obtained through an accessor
> >>> which would validate that the VCHIQ had been initialised
> >>> correctly with the remote.
> >>>
> >>> In commit 42a2f6664e18 ("staging: vc04_services: Move global g_state to
> >>> vchiq_state") the global state was moved to the vchiq_mgnt structures
> >>> stored as a vchiq instance specific context. This conversion removed the
> >>> helpers and instead replaced users of this helper with the assumption
> >>> that the state is always available and the remote connected.
> >>>
> >>> Fix this broken assumption by re-introducing the logic that was lost
> >>> during the conversion.
> >> thank you for sending this patch. Maybe it's worth to mention that this
> >> patch also drop some unnecessary NULL checks of state.
> > I don't understand this comment. Nothing is dropped is it?
> >
> > The newly added vchiq_remote_initialised() is itself a null-check too!
> the vchiq_remote_initialised() only checks the member remote, but not
> state itself. From my point of view the null-check for state is
> unnecessary, because most of the code already assumed that state is not
> null like e.g. in vchiq_open().
>

aha, I misread my own code ;-) Of course - I see it now.

I'll send v3.

--
Kieran