Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1557736 > unrolled thread

[patch net-next] stmmac: indent an if statement

Started byDan Carpenter <dan.carpenter@oracle.com>
First post2017-01-12 20:00 +0100
Last post2017-01-17 12:50 +0100
Articles 12 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [patch net-next] stmmac: indent an if statement Dan Carpenter <dan.carpenter@oracle.com> - 2017-01-12 20:00 +0100
    Re: [patch net-next] stmmac: indent an if statement Julia Lawall <julia.lawall@lip6.fr> - 2017-01-12 20:50 +0100
    Re: [patch net-next] stmmac: indent an if statement David Miller <davem@davemloft.net> - 2017-01-16 04:20 +0100
      Re: [patch net-next] stmmac: indent an if statement Dan Carpenter <dan.carpenter@oracle.com> - 2017-01-16 10:30 +0100
        Re: [patch net-next] stmmac: indent an if statement Dan Carpenter <dan.carpenter@oracle.com> - 2017-01-16 10:50 +0100
          Re: [patch net-next] stmmac: indent an if statement Julia Lawall <julia.lawall@lip6.fr> - 2017-01-16 22:50 +0100
            Re: [patch net-next] stmmac: indent an if statement Dan Carpenter <dan.carpenter@oracle.com> - 2017-01-16 23:00 +0100
              Re: [patch net-next] stmmac: indent an if statement David Miller <davem@davemloft.net> - 2017-01-16 23:10 +0100
                Re: [patch net-next] stmmac: indent an if statement Alexandre Torgue <alexandre.torgue@st.com> - 2017-01-17 09:30 +0100
              Re: [patch net-next] stmmac: indent an if statement Julia Lawall <julia.lawall@lip6.fr> - 2017-01-16 23:20 +0100
                Re: [patch net-next] stmmac: indent an if statement Alexandre Torgue <alexandre.torgue@st.com> - 2017-01-17 09:30 +0100
                  Re: [patch net-next] stmmac: indent an if statement Julia Lawall <julia.lawall@lip6.fr> - 2017-01-17 12:50 +0100

#1557736 — [patch net-next] stmmac: indent an if statement

FromDan Carpenter <dan.carpenter@oracle.com>
Date2017-01-12 20:00 +0100
Subject[patch net-next] stmmac: indent an if statement
Message-ID<sYSLo-7sp-27@gated-at.bofh.it>
The break statement should be indented one more tab.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index ac32f9e..4daa8a3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -191,7 +191,7 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
 		for_each_child_of_node(np, plat->mdio_node) {
 			if (of_device_is_compatible(plat->mdio_node,
 						    "snps,dwmac-mdio"))
-			break;
+				break;
 		}
 	}

[toc] | [next] | [standalone]

#1557751

FromJulia Lawall <julia.lawall@lip6.fr>
Date2017-01-12 20:50 +0100
Message-ID<sYTxM-7XH-13@gated-at.bofh.it>
In reply to#1557736

On Thu, 12 Jan 2017, Dan Carpenter wrote:

> The break statement should be indented one more tab.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index ac32f9e..4daa8a3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -191,7 +191,7 @@ static int stmmac_dt_phy(struct plat_stmmacenet_data *plat,
>  		for_each_child_of_node(np, plat->mdio_node) {
>  			if (of_device_is_compatible(plat->mdio_node,
>  						    "snps,dwmac-mdio"))
> -			break;
> +				break;

If there is a break, there is probably also a need for an of_node_put?

julia

>  		}
>  	}
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[toc] | [prev] | [next] | [standalone]

#1559415

FromDavid Miller <davem@davemloft.net>
Date2017-01-16 04:20 +0100
Message-ID<t05ZU-2O6-9@gated-at.bofh.it>
In reply to#1557736
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Thu, 12 Jan 2017 21:46:32 +0300

> The break statement should be indented one more tab.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied, but like Julia I think we might have a missing of_node_put()
here.

[toc] | [prev] | [next] | [standalone]

#1559547

FromDan Carpenter <dan.carpenter@oracle.com>
Date2017-01-16 10:30 +0100
Message-ID<t0bLY-6Q2-13@gated-at.bofh.it>
In reply to#1559415
On Sun, Jan 15, 2017 at 10:14:38PM -0500, David Miller wrote:
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Date: Thu, 12 Jan 2017 21:46:32 +0300
> 
> > The break statement should be indented one more tab.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Applied, but like Julia I think we might have a missing of_node_put()
> here.

Of course, sorry for dropping the ball on this.  I'll send a patch for
that.

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1559564

FromDan Carpenter <dan.carpenter@oracle.com>
Date2017-01-16 10:50 +0100
Message-ID<t0c5j-6Xo-11@gated-at.bofh.it>
In reply to#1559547
On Mon, Jan 16, 2017 at 12:19:24PM +0300, Dan Carpenter wrote:
> On Sun, Jan 15, 2017 at 10:14:38PM -0500, David Miller wrote:
> > From: Dan Carpenter <dan.carpenter@oracle.com>
> > Date: Thu, 12 Jan 2017 21:46:32 +0300
> > 
> > > The break statement should be indented one more tab.
> > > 
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > Applied, but like Julia I think we might have a missing of_node_put()
> > here.
> 
> Of course, sorry for dropping the ball on this.  I'll send a patch for
> that.
> 

Actually, I've looked at it some more and I think this function is OK.
We're supposed to do an of_node_put() later...  I can't find where that
happens, but presumably that's because I don't know stmmac well.  This
code here, though, is fine.

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1560120

FromJulia Lawall <julia.lawall@lip6.fr>
Date2017-01-16 22:50 +0100
Message-ID<t0nk6-6qK-17@gated-at.bofh.it>
In reply to#1559564

On Mon, 16 Jan 2017, Dan Carpenter wrote:

> On Mon, Jan 16, 2017 at 12:19:24PM +0300, Dan Carpenter wrote:
> > On Sun, Jan 15, 2017 at 10:14:38PM -0500, David Miller wrote:
> > > From: Dan Carpenter <dan.carpenter@oracle.com>
> > > Date: Thu, 12 Jan 2017 21:46:32 +0300
> > >
> > > > The break statement should be indented one more tab.
> > > >
> > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > >
> > > Applied, but like Julia I think we might have a missing of_node_put()
> > > here.
> >
> > Of course, sorry for dropping the ball on this.  I'll send a patch for
> > that.
> >
>
> Actually, I've looked at it some more and I think this function is OK.
> We're supposed to do an of_node_put() later...  I can't find where that
> happens, but presumably that's because I don't know stmmac well.  This
> code here, though, is fine.

Why do you think it is fine?  Does anyone in the calling context know
which child would have caused the break?  An extra put is only needed on
that one.  Is there a guarantee that the break is always taken?

julia

[toc] | [prev] | [next] | [standalone]

#1560130

FromDan Carpenter <dan.carpenter@oracle.com>
Date2017-01-16 23:00 +0100
Message-ID<t0ntN-6uv-35@gated-at.bofh.it>
In reply to#1560120
On Mon, Jan 16, 2017 at 10:46:22PM +0100, Julia Lawall wrote:
> 
> 
> On Mon, 16 Jan 2017, Dan Carpenter wrote:
> 
> > On Mon, Jan 16, 2017 at 12:19:24PM +0300, Dan Carpenter wrote:
> > > On Sun, Jan 15, 2017 at 10:14:38PM -0500, David Miller wrote:
> > > > From: Dan Carpenter <dan.carpenter@oracle.com>
> > > > Date: Thu, 12 Jan 2017 21:46:32 +0300
> > > >
> > > > > The break statement should be indented one more tab.
> > > > >
> > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > >
> > > > Applied, but like Julia I think we might have a missing of_node_put()
> > > > here.
> > >
> > > Of course, sorry for dropping the ball on this.  I'll send a patch for
> > > that.
> > >
> >
> > Actually, I've looked at it some more and I think this function is OK.
> > We're supposed to do an of_node_put() later...  I can't find where that
> > happens, but presumably that's because I don't know stmmac well.  This
> > code here, though, is fine.
> 
> Why do you think it is fine?  Does anyone in the calling context know
> which child would have caused the break?

Yeah.  It's saved in plat->mdio_node and we expect to be holding on
either path through the function.

(It would be better if one of the stmmac people were responding here
insead of a random fix the indenting weenie like myself.)

regards,
dan caprenter

[toc] | [prev] | [next] | [standalone]

#1560134

FromDavid Miller <davem@davemloft.net>
Date2017-01-16 23:10 +0100
Message-ID<t0nDs-6Nj-11@gated-at.bofh.it>
In reply to#1560130
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Tue, 17 Jan 2017 00:56:15 +0300

> (It would be better if one of the stmmac people were responding here
> insead of a random fix the indenting weenie like myself.)

They are all too busy trying to rename the driver, because that's so
much more important.

[toc] | [prev] | [next] | [standalone]

#1560340

FromAlexandre Torgue <alexandre.torgue@st.com>
Date2017-01-17 09:30 +0100
Message-ID<t0xjr-4DL-7@gated-at.bofh.it>
In reply to#1560134
Dear David

On 01/16/2017 11:00 PM, David Miller wrote:
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Date: Tue, 17 Jan 2017 00:56:15 +0300
>
>> (It would be better if one of the stmmac people were responding here
>> insead of a random fix the indenting weenie like myself.)
>
> They are all too busy trying to rename the driver, because that's so
> much more important.

No, we don't spend all our times to deals with stmmac renaming. Just 
busy on other topic and we continue to do our best with Peppe to review 
stmmac patch.

Regards
Alexandre

>

[toc] | [prev] | [next] | [standalone]

#1560136

FromJulia Lawall <julia.lawall@lip6.fr>
Date2017-01-16 23:20 +0100
Message-ID<t0nN7-6R9-5@gated-at.bofh.it>
In reply to#1560130

On Tue, 17 Jan 2017, Dan Carpenter wrote:

> On Mon, Jan 16, 2017 at 10:46:22PM +0100, Julia Lawall wrote:
> >
> >
> > On Mon, 16 Jan 2017, Dan Carpenter wrote:
> >
> > > On Mon, Jan 16, 2017 at 12:19:24PM +0300, Dan Carpenter wrote:
> > > > On Sun, Jan 15, 2017 at 10:14:38PM -0500, David Miller wrote:
> > > > > From: Dan Carpenter <dan.carpenter@oracle.com>
> > > > > Date: Thu, 12 Jan 2017 21:46:32 +0300
> > > > >
> > > > > > The break statement should be indented one more tab.
> > > > > >
> > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > >
> > > > > Applied, but like Julia I think we might have a missing of_node_put()
> > > > > here.
> > > >
> > > > Of course, sorry for dropping the ball on this.  I'll send a patch for
> > > > that.
> > > >
> > >
> > > Actually, I've looked at it some more and I think this function is OK.
> > > We're supposed to do an of_node_put() later...  I can't find where that
> > > happens, but presumably that's because I don't know stmmac well.  This
> > > code here, though, is fine.
> >
> > Why do you think it is fine?  Does anyone in the calling context know
> > which child would have caused the break?
>
> Yeah.  It's saved in plat->mdio_node and we expect to be holding on
> either path through the function.
>
> (It would be better if one of the stmmac people were responding here
> insead of a random fix the indenting weenie like myself.)

OK, I agree that there should not be an of_node_put with the break.

Perhaps there should be an of_node_put on plat->mdio_node in
stmmac_remove_config_dt, like there is an of_node_put on plat->phy_node.
But it would certainly be helpful to hear from someone who knows the code
better.

julia

[toc] | [prev] | [next] | [standalone]

#1560341

FromAlexandre Torgue <alexandre.torgue@st.com>
Date2017-01-17 09:30 +0100
Message-ID<t0xjs-4DL-15@gated-at.bofh.it>
In reply to#1560136
Hi Julia

On 01/16/2017 11:10 PM, Julia Lawall wrote:
>
>
> On Tue, 17 Jan 2017, Dan Carpenter wrote:
>
>> On Mon, Jan 16, 2017 at 10:46:22PM +0100, Julia Lawall wrote:
>>>
>>>
>>> On Mon, 16 Jan 2017, Dan Carpenter wrote:
>>>
>>>> On Mon, Jan 16, 2017 at 12:19:24PM +0300, Dan Carpenter wrote:
>>>>> On Sun, Jan 15, 2017 at 10:14:38PM -0500, David Miller wrote:
>>>>>> From: Dan Carpenter <dan.carpenter@oracle.com>
>>>>>> Date: Thu, 12 Jan 2017 21:46:32 +0300
>>>>>>
>>>>>>> The break statement should be indented one more tab.
>>>>>>>
>>>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>>>
>>>>>> Applied, but like Julia I think we might have a missing of_node_put()
>>>>>> here.
>>>>>
>>>>> Of course, sorry for dropping the ball on this.  I'll send a patch for
>>>>> that.
>>>>>
>>>>
>>>> Actually, I've looked at it some more and I think this function is OK.
>>>> We're supposed to do an of_node_put() later...  I can't find where that
>>>> happens, but presumably that's because I don't know stmmac well.  This
>>>> code here, though, is fine.
>>>
>>> Why do you think it is fine?  Does anyone in the calling context know
>>> which child would have caused the break?
>>
>> Yeah.  It's saved in plat->mdio_node and we expect to be holding on
>> either path through the function.
>>
>> (It would be better if one of the stmmac people were responding here
>> insead of a random fix the indenting weenie like myself.)
>
> OK, I agree that there should not be an of_node_put with the break.
>
> Perhaps there should be an of_node_put on plat->mdio_node in
> stmmac_remove_config_dt, like there is an of_node_put on plat->phy_node.
> But it would certainly be helpful to hear from someone who knows the code
> better.

I also think it's missing! Can you propose a patch ?

br
Alex

>
> julia
>

[toc] | [prev] | [next] | [standalone]

#1560526

FromJulia Lawall <julia.lawall@lip6.fr>
Date2017-01-17 12:50 +0100
Message-ID<t0AqZ-6A1-5@gated-at.bofh.it>
In reply to#1560341

On Tue, 17 Jan 2017, Alexandre Torgue wrote:

> Hi Julia
>
> On 01/16/2017 11:10 PM, Julia Lawall wrote:
> >
> >
> > On Tue, 17 Jan 2017, Dan Carpenter wrote:
> >
> > > On Mon, Jan 16, 2017 at 10:46:22PM +0100, Julia Lawall wrote:
> > > >
> > > >
> > > > On Mon, 16 Jan 2017, Dan Carpenter wrote:
> > > >
> > > > > On Mon, Jan 16, 2017 at 12:19:24PM +0300, Dan Carpenter wrote:
> > > > > > On Sun, Jan 15, 2017 at 10:14:38PM -0500, David Miller wrote:
> > > > > > > From: Dan Carpenter <dan.carpenter@oracle.com>
> > > > > > > Date: Thu, 12 Jan 2017 21:46:32 +0300
> > > > > > >
> > > > > > > > The break statement should be indented one more tab.
> > > > > > > >
> > > > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > > > >
> > > > > > > Applied, but like Julia I think we might have a missing
> > > > > > > of_node_put()
> > > > > > > here.
> > > > > >
> > > > > > Of course, sorry for dropping the ball on this.  I'll send a patch
> > > > > > for
> > > > > > that.
> > > > > >
> > > > >
> > > > > Actually, I've looked at it some more and I think this function is OK.
> > > > > We're supposed to do an of_node_put() later...  I can't find where
> > > > > that
> > > > > happens, but presumably that's because I don't know stmmac well.  This
> > > > > code here, though, is fine.
> > > >
> > > > Why do you think it is fine?  Does anyone in the calling context know
> > > > which child would have caused the break?
> > >
> > > Yeah.  It's saved in plat->mdio_node and we expect to be holding on
> > > either path through the function.
> > >
> > > (It would be better if one of the stmmac people were responding here
> > > insead of a random fix the indenting weenie like myself.)
> >
> > OK, I agree that there should not be an of_node_put with the break.
> >
> > Perhaps there should be an of_node_put on plat->mdio_node in
> > stmmac_remove_config_dt, like there is an of_node_put on plat->phy_node.
> > But it would certainly be helpful to hear from someone who knows the code
> > better.
>
> I also think it's missing! Can you propose a patch ?

Done.  Thanks for the clarification.

julia

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web