| Message ID | 20230710063933.172926-1-suhui@nfschina.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Remove unnecessary (void*) conversions | expand |
On Mon, Jul 10, 2023 at 02:39:33PM +0800, Su Hui wrote: > From: wuych <yunchuan@nfschina.com> ^^^^^ This doesn't look like a real name. > > Pointer variables of void * type do not require type cast. > > Signed-off-by: wuych <yunchuan@nfschina.com> > --- > 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
On 2023/7/10 15:29, Dan Carpenter wrote: > On Mon, Jul 10, 2023 at 02:39:33PM +0800, Su Hui wrote: >> From: wuych <yunchuan@nfschina.com> > ^^^^^ > 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 <yunchuan@nfschina.com> >> --- >> 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 >
On Mon, Jul 10, 2023 at 02:39:33PM +0800, Su Hui wrote: > From: wuych <yunchuan@nfschina.com> > > Pointer variables of void * type do not require type cast. > > Signed-off-by: wuych <yunchuan@nfschina.com> > --- > 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
On 2023/7/11 00:34, Andrew Lunn wrote: > On Mon, Jul 10, 2023 at 02:39:33PM +0800, Su Hui wrote: >> From: wuych <yunchuan@nfschina.com> >> >> Pointer variables of void * type do not require type cast. >> >> Signed-off-by: wuych <yunchuan@nfschina.com> >> --- >> 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
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: