Re: [PATCH net-next v2 01/10] net: wan: Remove unnecessary (void*) conversions

[yunchuan]

On 2023/7/11 00:34, Andrew Lunn wrote:

On Mon, Jul 10, 2023 at 02:39:33PM +0800, Su Hui wrote:

From: wuych 

Pointer variables of void * type do not require type cast.

Signed-off-by: wuych 
---
  drivers/net/wan/fsl_ucc_hdlc.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 47c2ad7a3e42..73c73d8f4bb2 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -350,11 +350,11 @@ static int uhdlc_init(struct ucc_hdlc_private *priv)
  static netdev_tx_t ucc_hdlc_tx(struct sk_buff *skb, struct net_device *dev)
  {
hdlc_device *hdlc = dev_to_hdlc(dev);
-   struct ucc_hdlc_private *priv = (struct ucc_hdlc_private *)hdlc->priv;
-   struct qe_bd *bd;
-   u16 bd_status;
+   struct ucc_hdlc_private *priv = hdlc->priv;
unsigned long flags;
__be16 *proto_head;
+   struct qe_bd *bd;
+   u16 bd_status;

When dealing with existing broken reverse Christmas tree, please don't
make it worse with a change. But actually fixing it should be in a
different patch.

We want patches to be obviously correct. By removing the cast and
moving variables around, it is less obvious it is correct, than having
two patches.

Got it, thanks.
I will resend the v3 later  which remove the change of reverse Christmas 
tree.

So sorry for this!

Wu Yunchuan



Andrew

Re: [PATCH net-next v2 01/10] net: wan: Remove unnecessary (void*) conversions

[Andrew Lunn]

On Mon, Jul 10, 2023 at 02:39:33PM +0800, Su Hui wrote:
> From: wuych 
> 
> Pointer variables of void * type do not require type cast.
> 
> Signed-off-by: wuych 
> ---
>  drivers/net/wan/fsl_ucc_hdlc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
> index 47c2ad7a3e42..73c73d8f4bb2 100644
> --- a/drivers/net/wan/fsl_ucc_hdlc.c
> +++ b/drivers/net/wan/fsl_ucc_hdlc.c
> @@ -350,11 +350,11 @@ static int uhdlc_init(struct ucc_hdlc_private *priv)
>  static netdev_tx_t ucc_hdlc_tx(struct sk_buff *skb, struct net_device *dev)
>  {
>   hdlc_device *hdlc = dev_to_hdlc(dev);
> - struct ucc_hdlc_private *priv = (struct ucc_hdlc_private *)hdlc->priv;
> - struct qe_bd *bd;
> - u16 bd_status;
> + struct ucc_hdlc_private *priv = hdlc->priv;
>   unsigned long flags;
>   __be16 *proto_head;
> + struct qe_bd *bd;
> + u16 bd_status;

When dealing with existing broken reverse Christmas tree, please don't
make it worse with a change. But actually fixing it should be in a
different patch.

We want patches to be obviously correct. By removing the cast and
moving variables around, it is less obvious it is correct, than having
two patches.

   Andrew

Re: [PATCH net-next v2 01/10] net: wan: Remove unnecessary (void*) conversions

[yunchuan]

On 2023/7/10 15:29, Dan Carpenter wrote:

On Mon, Jul 10, 2023 at 02:39:33PM +0800, Su Hui wrote:

From: wuych 

 ^
This doesn't look like a real name.


Hi, Dan,

My full name is wuyunchan， but that is too long.
So I use the wuych as simplification.
I will change it to my full name, thanks for your adivce!



Pointer variables of void * type do not require type cast.

Signed-off-by: wuych 
---
  drivers/net/wan/fsl_ucc_hdlc.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 47c2ad7a3e42..73c73d8f4bb2 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -350,11 +350,11 @@ static int uhdlc_init(struct ucc_hdlc_private *priv)
  static netdev_tx_t ucc_hdlc_tx(struct sk_buff *skb, struct net_device *dev)
  {
hdlc_device *hdlc = dev_to_hdlc(dev);
-   struct ucc_hdlc_private *priv = (struct ucc_hdlc_private *)hdlc->priv;
-   struct qe_bd *bd;
-   u16 bd_status;
+   struct ucc_hdlc_private *priv = hdlc->priv;
unsigned long flags;
__be16 *proto_head;
+   struct qe_bd *bd;
+   u16 bd_status;

Don't move the other variables around.  That's unrelated to the cast.
(Same applies to all the other patches).


I want to move the declarations to be reverse xmas tree.
Maybe moving them is more better?

wuych



regards,
dan carpenter

Re: [PATCH net-next v2 01/10] net: wan: Remove unnecessary (void*) conversions

[Dan Carpenter]

On Mon, Jul 10, 2023 at 02:39:33PM +0800, Su Hui wrote:
> From: wuych 
^
This doesn't look like a real name.

> 
> Pointer variables of void * type do not require type cast.
> 
> Signed-off-by: wuych 
> ---
>  drivers/net/wan/fsl_ucc_hdlc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
> index 47c2ad7a3e42..73c73d8f4bb2 100644
> --- a/drivers/net/wan/fsl_ucc_hdlc.c
> +++ b/drivers/net/wan/fsl_ucc_hdlc.c
> @@ -350,11 +350,11 @@ static int uhdlc_init(struct ucc_hdlc_private *priv)
>  static netdev_tx_t ucc_hdlc_tx(struct sk_buff *skb, struct net_device *dev)
>  {
>   hdlc_device *hdlc = dev_to_hdlc(dev);
> - struct ucc_hdlc_private *priv = (struct ucc_hdlc_private *)hdlc->priv;
> - struct qe_bd *bd;
> - u16 bd_status;
> + struct ucc_hdlc_private *priv = hdlc->priv;
>   unsigned long flags;
>   __be16 *proto_head;
> + struct qe_bd *bd;
> + u16 bd_status;

Don't move the other variables around.  That's unrelated to the cast.
(Same applies to all the other patches).

regards,
dan carpenter

[PATCH net-next v2 01/10] net: wan: Remove unnecessary (void*) conversions

[Su Hui]

From: wuych 

Pointer variables of void * type do not require type cast.

Signed-off-by: wuych 
---
 drivers/net/wan/fsl_ucc_hdlc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 47c2ad7a3e42..73c73d8f4bb2 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -350,11 +350,11 @@ static int uhdlc_init(struct ucc_hdlc_private *priv)
 static netdev_tx_t ucc_hdlc_tx(struct sk_buff *skb, struct net_device *dev)
 {
hdlc_device *hdlc = dev_to_hdlc(dev);
-   struct ucc_hdlc_private *priv = (struct ucc_hdlc_private *)hdlc->priv;
-   struct qe_bd *bd;
-   u16 bd_status;
+   struct ucc_hdlc_private *priv = hdlc->priv;
unsigned long flags;
__be16 *proto_head;
+   struct qe_bd *bd;
+   u16 bd_status;
 
switch (dev->type) {
case ARPHRD_RAWHDLC:
-- 
2.30.2