Re: FWD: [PATCH 3/3] net: phylink: correct trivial kernel-doc inconsistencies

Re: FWD: [PATCH 3/3] net: phylink: correct trivial kernel-doc inconsistencies

[Russell King - ARM Linux admin]

On Sun, Jun 21, 2020 at 05:42:48PM +0200, Andrew Lunn wrote:
> FYI:
> 
> 	Andrew

Thanks for forwarding - Colton, please copy me directly.

> 
> ----- Forwarded message from Colton Lewis <colton.w.lewis@protonmail.com> -----
> 
> Date: Sun, 21 Jun 2020 02:23:04 +0000
> From: Colton Lewis <colton.w.lewis@protonmail.com>
> To: davem@davemloft.net
> Cc: netdev@vger.kernel.org, Colton Lewis <colton.w.lewis@protonmail.com>
> Subject: [PATCH 3/3] net: phylink: correct trivial kernel-doc inconsistencies
> X-Spam-Status: No, score=-1.0 required=4.0 tests=BAYES_00,DKIM_SIGNED,
> 	DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,
> 	HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_PASS, SPF_PASS autolearn=no autolearn_force=no
> 	version=3.4.4
> 
> Silence documentation build warnings by correcting kernel-doc
> comments. In the case of pcs_{config,an_restart,link_up}, change the
> declaration to a normal function since these only there for
> documentation anyway.
> 
> ./include/linux/phylink.h:74: warning: Function parameter or member 'poll_fixed_state' not described in 'phylink_config'
> ./include/linux/phylink.h:74: warning: Function parameter or member 'get_fixed_state' not described in 'phylink_config'
> ./include/linux/phylink.h:336: warning: Function parameter or member 'pcs_config' not described in 'int'
> ./include/linux/phylink.h:336: warning: Excess function parameter 'config' description in 'int'
> ./include/linux/phylink.h:336: warning: Excess function parameter 'mode' description in 'int'
> ./include/linux/phylink.h:336: warning: Excess function parameter 'interface' description in 'int'
> ./include/linux/phylink.h:336: warning: Excess function parameter 'advertising' description in 'int'
> ./include/linux/phylink.h:345: warning: Function parameter or member 'pcs_an_restart' not described in 'void'
> ./include/linux/phylink.h:345: warning: Excess function parameter 'config' description in 'void'
> ./include/linux/phylink.h:361: warning: Function parameter or member 'pcs_link_up' not described in 'void'
> ./include/linux/phylink.h:361: warning: Excess function parameter 'config' description in 'void'
> ./include/linux/phylink.h:361: warning: Excess function parameter 'mode' description in 'void'
> ./include/linux/phylink.h:361: warning: Excess function parameter 'interface' description in 'void'
> ./include/linux/phylink.h:361: warning: Excess function parameter 'speed' description in 'void'
> ./include/linux/phylink.h:361: warning: Excess function parameter 'duplex' description in 'void'

Not all of those are valid complaints against the code.

> Signed-off-by: Colton Lewis <colton.w.lewis@protonmail.com>
> ---
>  include/linux/phylink.h | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index cc5b452a184e..cb3230590a1f 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -62,6 +62,8 @@ enum phylink_op_type {
>   * @dev: a pointer to a struct device associated with the MAC
>   * @type: operation type of PHYLINK instance
>   * @pcs_poll: MAC PCS cannot provide link change interrupt
> + * @poll_fixed_state: poll link state with @get_fixed_state
> + * @get_fixed_state: read link state into struct phylink_link_state

Ack for this change.

>   */
>  struct phylink_config {
>  	struct device *dev;
> @@ -331,7 +333,7 @@ void pcs_get_state(struct phylink_config *config,
>   *
>   * For most 10GBASE-R, there is no advertisement.
>   */
> -int (*pcs_config)(struct phylink_config *config, unsigned int mode,
> +int *pcs_config(struct phylink_config *config, unsigned int mode,
>  		  phy_interface_t interface, const unsigned long *advertising);

*Definitely* a NAK on this and two changes below.  You're changing the
function signature to be incorrect.  If the documentation can't parse
a legitimate C function pointer declaration and allow it to be
documented, then that's a problem with the documentation's parsing of
C code, rather than a problem with the C code itself.

>  
>  /**
> @@ -341,7 +343,7 @@ int (*pcs_config)(struct phylink_config *config, unsigned int mode,
>   * When PCS ops are present, this overrides mac_an_restart() in &struct
>   * phylink_mac_ops.
>   */
> -void (*pcs_an_restart)(struct phylink_config *config);
> +void *pcs_an_restart(struct phylink_config *config);
>  
>  /**
>   * pcs_link_up() - program the PCS for the resolved link configuration
> @@ -356,7 +358,7 @@ void (*pcs_an_restart)(struct phylink_config *config);
>   * mode without in-band AN needs to be manually configured for the link
>   * and duplex setting. Otherwise, this should be a no-op.
>   */
> -void (*pcs_link_up)(struct phylink_config *config, unsigned int mode,
> +void *pcs_link_up(struct phylink_config *config, unsigned int mode,
>  		    phy_interface_t interface, int speed, int duplex);
>  #endif
>  
> -- 
> 2.26.2
> 
> 
> 
> ----- End forwarded message -----
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: FWD: [PATCH 3/3] net: phylink: correct trivial kernel-doc inconsistencies

[Colton Lewis]

On Sunday, June 21, 2020 10:53:45 AM CDT Russell King - ARM Linux admin wrote:
> > ---
> >   */
> >  struct phylink_config {
> >  	struct device *dev;
> > @@ -331,7 +333,7 @@ void pcs_get_state(struct phylink_config *config,
> >   *
> >   * For most 10GBASE-R, there is no advertisement.
> >   */
> > -int (*pcs_config)(struct phylink_config *config, unsigned int mode,
> > +int *pcs_config(struct phylink_config *config, unsigned int mode,
> >  		  phy_interface_t interface, const unsigned long *advertising);
> 
> *Definitely* a NAK on this and two changes below.  You're changing the
> function signature to be incorrect.  If the documentation can't parse
> a legitimate C function pointer declaration and allow it to be
> documented, then that's a problem with the documentation's parsing of
> C code, rather than a problem with the C code itself.

I realize this changes the signature, but this declaration is not compiled. It is under an #if 0 with a comment stating it exists for kernel-doc purposes only. The *real* function pointer declaration exists in struct phylink_pcs_ops.

Given the declaration is there exclusively for documentation, it makes sense to change it so the documentation system can parse it.

Re: FWD: [PATCH 3/3] net: phylink: correct trivial kernel-doc inconsistencies

[Russell King - ARM Linux admin]

On Sun, Jun 21, 2020 at 11:02:30PM +0000, Colton Lewis wrote:
> On Sunday, June 21, 2020 10:53:45 AM CDT Russell King - ARM Linux admin wrote:
> > > ---
> > >   */
> > >  struct phylink_config {
> > >  	struct device *dev;
> > > @@ -331,7 +333,7 @@ void pcs_get_state(struct phylink_config *config,
> > >   *
> > >   * For most 10GBASE-R, there is no advertisement.
> > >   */
> > > -int (*pcs_config)(struct phylink_config *config, unsigned int mode,
> > > +int *pcs_config(struct phylink_config *config, unsigned int mode,
> > >  		  phy_interface_t interface, const unsigned long *advertising);
> > 
> > *Definitely* a NAK on this and two changes below.  You're changing the
> > function signature to be incorrect.  If the documentation can't parse
> > a legitimate C function pointer declaration and allow it to be
> > documented, then that's a problem with the documentation's parsing of
> > C code, rather than a problem with the C code itself.
> 
> I realize this changes the signature, but this declaration is not compiled. It is under an #if 0 with a comment stating it exists for kernel-doc purposes only. The *real* function pointer declaration exists in struct phylink_pcs_ops.
> 
> Given the declaration is there exclusively for documentation, it makes sense to change it so the documentation system can parse it.

My objection is that you are changing the return type from (e.g.)
"int" to "int *", which will then end up in the documentation as
such, and the documentation will, therefore, be incorrect.

I have subsequently realised that I didn't follow my own pattern
for documenting phylink_mac_ops - a correct solution would be to
drop the parens _and_ the "*" preceding the function name, so:

int pcs_config(struct phylink_config *config, unsigned int mode,
...

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 3/3] net: phylink: correct trivial kernel-doc inconsistencies

[David Miller]

From: Colton Lewis <colton.w.lewis@protonmail.com>
Date: Sun, 21 Jun 2020 23:02:30 +0000

> On Sunday, June 21, 2020 10:53:45 AM CDT Russell King - ARM Linux admin wrote:
>> > ---
>> >   */
>> >  struct phylink_config {
>> >  	struct device *dev;
>> > @@ -331,7 +333,7 @@ void pcs_get_state(struct phylink_config *config,
>> >   *
>> >   * For most 10GBASE-R, there is no advertisement.
>> >   */
>> > -int (*pcs_config)(struct phylink_config *config, unsigned int mode,
>> > +int *pcs_config(struct phylink_config *config, unsigned int mode,
>> >  		  phy_interface_t interface, const unsigned long *advertising);
>> 
>> *Definitely* a NAK on this and two changes below.  You're changing the
>> function signature to be incorrect.  If the documentation can't parse
>> a legitimate C function pointer declaration and allow it to be
>> documented, then that's a problem with the documentation's parsing of
>> C code, rather than a problem with the C code itself.
> 
> I realize this changes the signature, but this declaration is not compiled. It is under an #if 0 with a comment stating it exists for kernel-doc purposes only. The *real* function pointer declaration exists in struct phylink_pcs_ops.
> 
> Given the declaration is there exclusively for documentation, it makes sense to change it so the documentation system can parse it.

I agree with Russell, if the C code can't be accurately represented you
make things worse for people trying to actually _use_ the documentation.

Can't you escape the parenthesis or something like that?

If you can't make it look accurate, leave it alone.

[PATCH v3] net: phylink: correct trivial kernel-doc inconsistencies

[Colton Lewis]

Silence documentation build warnings by correcting kernel-doc
comments. In the case of pcs_{config,an_restart,link_up}, change the
declaration to a normal function since these only there for
documentation anyway.

./include/linux/phylink.h:74: warning: Function parameter or member 'poll_fixed_state' not described in 'phylink_config'
./include/linux/phylink.h:74: warning: Function parameter or member 'get_fixed_state' not described in 'phylink_config'
./include/linux/phylink.h:336: warning: Function parameter or member 'pcs_config' not described in 'int'
./include/linux/phylink.h:336: warning: Excess function parameter 'config' description in 'int'
./include/linux/phylink.h:336: warning: Excess function parameter 'mode' description in 'int'
./include/linux/phylink.h:336: warning: Excess function parameter 'interface' description in 'int'
./include/linux/phylink.h:336: warning: Excess function parameter 'advertising' description in 'int'
./include/linux/phylink.h:345: warning: Function parameter or member 'pcs_an_restart' not described in 'void'
./include/linux/phylink.h:345: warning: Excess function parameter 'config' description in 'void'
./include/linux/phylink.h:361: warning: Function parameter or member 'pcs_link_up' not described in 'void'
./include/linux/phylink.h:361: warning: Excess function parameter 'config' description in 'void'
./include/linux/phylink.h:361: warning: Excess function parameter 'mode' description in 'void'
./include/linux/phylink.h:361: warning: Excess function parameter 'interface' description in 'void'
./include/linux/phylink.h:361: warning: Excess function parameter 'speed' description in 'void'
./include/linux/phylink.h:361: warning: Excess function parameter 'duplex' description in 'void'

Signed-off-by: Colton Lewis <colton.w.lewis@protonmail.com>
---
 include/linux/phylink.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index cc5b452a184e..24c52d9f63d6 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -62,6 +62,8 @@ enum phylink_op_type {
  * @dev: a pointer to a struct device associated with the MAC
  * @type: operation type of PHYLINK instance
  * @pcs_poll: MAC PCS cannot provide link change interrupt
+ * @poll_fixed_state: poll link state with @get_fixed_state
+ * @get_fixed_state: read link state into struct phylink_link_state
  */
 struct phylink_config {
 	struct device *dev;
@@ -331,7 +333,7 @@ void pcs_get_state(struct phylink_config *config,
  *
  * For most 10GBASE-R, there is no advertisement.
  */
-int (*pcs_config)(struct phylink_config *config, unsigned int mode,
+int pcs_config(struct phylink_config *config, unsigned int mode,
 		  phy_interface_t interface, const unsigned long *advertising);
 
 /**
@@ -341,7 +343,7 @@ int (*pcs_config)(struct phylink_config *config, unsigned int mode,
  * When PCS ops are present, this overrides mac_an_restart() in &struct
  * phylink_mac_ops.
  */
-void (*pcs_an_restart)(struct phylink_config *config);
+void pcs_an_restart(struct phylink_config *config);
 
 /**
  * pcs_link_up() - program the PCS for the resolved link configuration
@@ -356,7 +358,7 @@ void (*pcs_an_restart)(struct phylink_config *config);
  * mode without in-band AN needs to be manually configured for the link
  * and duplex setting. Otherwise, this should be a no-op.
  */
-void (*pcs_link_up)(struct phylink_config *config, unsigned int mode,
+void pcs_link_up(struct phylink_config *config, unsigned int mode,
 		    phy_interface_t interface, int speed, int duplex);
 #endif
 
-- 
2.26.2

Re: [PATCH v3] net: phylink: correct trivial kernel-doc inconsistencies

[Russell King - ARM Linux admin]

On Sat, Jun 27, 2020 at 11:58:09PM +0000, Colton Lewis wrote:
> Silence documentation build warnings by correcting kernel-doc
> comments. In the case of pcs_{config,an_restart,link_up}, change the
> declaration to a normal function since these only there for
> documentation anyway.
> 
> ./include/linux/phylink.h:74: warning: Function parameter or member 'poll_fixed_state' not described in 'phylink_config'
> ./include/linux/phylink.h:74: warning: Function parameter or member 'get_fixed_state' not described in 'phylink_config'
> ./include/linux/phylink.h:336: warning: Function parameter or member 'pcs_config' not described in 'int'
> ./include/linux/phylink.h:336: warning: Excess function parameter 'config' description in 'int'
> ./include/linux/phylink.h:336: warning: Excess function parameter 'mode' description in 'int'
> ./include/linux/phylink.h:336: warning: Excess function parameter 'interface' description in 'int'
> ./include/linux/phylink.h:336: warning: Excess function parameter 'advertising' description in 'int'
> ./include/linux/phylink.h:345: warning: Function parameter or member 'pcs_an_restart' not described in 'void'
> ./include/linux/phylink.h:345: warning: Excess function parameter 'config' description in 'void'
> ./include/linux/phylink.h:361: warning: Function parameter or member 'pcs_link_up' not described in 'void'
> ./include/linux/phylink.h:361: warning: Excess function parameter 'config' description in 'void'
> ./include/linux/phylink.h:361: warning: Excess function parameter 'mode' description in 'void'
> ./include/linux/phylink.h:361: warning: Excess function parameter 'interface' description in 'void'
> ./include/linux/phylink.h:361: warning: Excess function parameter 'speed' description in 'void'
> ./include/linux/phylink.h:361: warning: Excess function parameter 'duplex' description in 'void'
> 
> Signed-off-by: Colton Lewis <colton.w.lewis@protonmail.com>
> ---
>  include/linux/phylink.h | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index cc5b452a184e..24c52d9f63d6 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -62,6 +62,8 @@ enum phylink_op_type {
>   * @dev: a pointer to a struct device associated with the MAC
>   * @type: operation type of PHYLINK instance
>   * @pcs_poll: MAC PCS cannot provide link change interrupt
> + * @poll_fixed_state: poll link state with @get_fixed_state
> + * @get_fixed_state: read link state into struct phylink_link_state
>   */
>  struct phylink_config {
>  	struct device *dev;
> @@ -331,7 +333,7 @@ void pcs_get_state(struct phylink_config *config,
>   *
>   * For most 10GBASE-R, there is no advertisement.
>   */
> -int (*pcs_config)(struct phylink_config *config, unsigned int mode,
> +int pcs_config(struct phylink_config *config, unsigned int mode,
>  		  phy_interface_t interface, const unsigned long *advertising);

We seem to be having a communication breakdown.  In review to your
version 2 patch set, I said:

   However, please drop all your changes for everything but the
   "struct phylink_config" documentation change; I'm intending to change
   all these method signatures, which means your changes will conflict.

But the changes still exist in version 3.  What gives?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v3] net: phylink: correct trivial kernel-doc inconsistencies

[Colton Lewis]

> We seem to be having a communication breakdown.  In review to your
> version 2 patch set, I said:
> 
>    However, please drop all your changes for everything but the
>    "struct phylink_config" documentation change; I'm intending to change
>    all these method signatures, which means your changes will conflict.
> 
> But the changes still exist in version 3.  What gives?

You said *drop all your changes* for *everything but* the struct phylink_config change. I interpreted this to mean you wanted *only* struct phylink_config. In context of your previous comments, I might have guessed you meant the opposite.

[PATCH v4 1/2] net: core: correct trivial kernel-doc inconsistencies

[Colton Lewis]

Silence documentation build warnings by correcting kernel-doc comments.

./net/core/dev.c:7913: warning: Function parameter or member 'dev' not described in 'netdev_get_xmit_slave'

Signed-off-by: Colton Lewis <colton.w.lewis@protonmail.com>
---
 net/core/dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6bc2388141f6..cf20d286abfc 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7898,6 +7898,7 @@ EXPORT_SYMBOL(netdev_bonding_info_change);
 
 /**
  * netdev_get_xmit_slave - Get the xmit slave of master device
+ * @dev: The device
  * @skb: The packet
  * @all_slaves: assume all the slaves are active
  *
-- 
2.26.2

[PATCH v4 2/2] net: core: correct trivial kernel-doc inconsistencies

[Colton Lewis]

Silence documentation build warnings by correcting kernel-doc comments.

./include/linux/netdevice.h:2138: warning: Function parameter or member 'napi_defer_hard_irqs' not described in 'net_device'

Signed-off-by: Colton Lewis <colton.w.lewis@protonmail.com>
---
 include/linux/netdevice.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 6fc613ed8eae..515791a9b299 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1742,6 +1742,7 @@ enum netdev_priv_flags {
  *	@real_num_rx_queues: 	Number of RX queues currently active in device
  *	@xdp_prog:		XDP sockets filter program pointer
  *	@gro_flush_timeout:	timeout for GRO layer in NAPI
+ *	@napi_defer_hard_irqs:	count of deferred hardware interrupt requests
  *
  *	@rx_handler:		handler for received packets
  *	@rx_handler_data: 	XXX: need comments on this one
-- 
2.26.2

Re: [PATCH v3] net: phylink: correct trivial kernel-doc inconsistencies

[Russell King - ARM Linux admin]

On Sun, Jun 28, 2020 at 09:36:35PM +0000, Colton Lewis wrote:
> > We seem to be having a communication breakdown.  In review to your
> > version 2 patch set, I said:
> > 
> >    However, please drop all your changes for everything but the
> >    "struct phylink_config" documentation change; I'm intending to change
> >    all these method signatures, which means your changes will conflict.
> > 
> > But the changes still exist in version 3.  What gives?
> 
> You said *drop all your changes* for *everything but* the struct phylink_config change. I interpreted this to mean you wanted *only* struct phylink_config. In context of your previous comments, I might have guessed you meant the opposite.

It seems we're using different versions of English, because your v4 is
still wrong.

Want I want for the phylink change is to see the hunk that changes
struct phylink_config ONLY and NOT any of the individual method
configuration.  In other words, I want:

diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index cc5b452a184e..cb3230590a1f 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -62,6 +62,8 @@ enum phylink_op_type {
  * @dev: a pointer to a struct device associated with the MAC
  * @type: operation type of PHYLINK instance
  * @pcs_poll: MAC PCS cannot provide link change interrupt
+ * @poll_fixed_state: poll link state with @get_fixed_state
+ * @get_fixed_state: read link state into struct phylink_link_state
  */
 struct phylink_config {
        struct device *dev;


But I don't want:

@@ -331,7 +333,7 @@ void pcs_get_state(struct phylink_config *config,
  *
  * For most 10GBASE-R, there is no advertisement.
  */
-int (*pcs_config)(struct phylink_config *config, unsigned int mode,
+int *pcs_config(struct phylink_config *config, unsigned int mode,
                  phy_interface_t interface, const unsigned long *advertising);

 /**

and the rest of those in that file.

I really don't think I could have been clearer without creating the
damn patch for you.

Second thoughts, don't bother, I'll do it myself, the amount of effort
wasted here is rediculous, and I really don't want to go round this
loop yet again.

Thanks for pointing the issues out.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v4 1/2] net: core: correct trivial kernel-doc inconsistencies

[David Miller]

From: Colton Lewis <colton.w.lewis@protonmail.com>
Date: Sun, 28 Jun 2020 21:39:32 +0000

> Silence documentation build warnings by correcting kernel-doc comments.
> 
> ./net/core/dev.c:7913: warning: Function parameter or member 'dev' not described in 'netdev_get_xmit_slave'
> 
> Signed-off-by: Colton Lewis <colton.w.lewis@protonmail.com>

Applied.

Re: [PATCH v4 2/2] net: core: correct trivial kernel-doc inconsistencies

[David Miller]

From: Colton Lewis <colton.w.lewis@protonmail.com>
Date: Sun, 28 Jun 2020 21:39:38 +0000

> Silence documentation build warnings by correcting kernel-doc comments.
> 
> ./include/linux/netdevice.h:2138: warning: Function parameter or member 'napi_defer_hard_irqs' not described in 'net_device'
> 
> Signed-off-by: Colton Lewis <colton.w.lewis@protonmail.com>

Applied.

Re: [PATCH v3] net: phylink: correct trivial kernel-doc inconsistencies

[David Miller]

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Date: Sun, 28 Jun 2020 23:20:36 +0100

> On Sun, Jun 28, 2020 at 09:36:35PM +0000, Colton Lewis wrote:
>> > We seem to be having a communication breakdown.  In review to your
>> > version 2 patch set, I said:
>> > 
>> >    However, please drop all your changes for everything but the
>> >    "struct phylink_config" documentation change; I'm intending to change
>> >    all these method signatures, which means your changes will conflict.
>> > 
>> > But the changes still exist in version 3.  What gives?
>> 
>> You said *drop all your changes* for *everything but* the struct phylink_config change. I interpreted this to mean you wanted *only* struct phylink_config. In context of your previous comments, I might have guessed you meant the opposite.
> 
> It seems we're using different versions of English, because your v4 is
> still wrong.

Aha, ok I'll revert then, thanks.