[net-next] net: ethernet: ti: cpdma: rate is not changed - correct case
diff mbox

Message ID 1512571278-13196-1-git-send-email-ivan.khoronzhuk@linaro.org
State New
Headers show

Commit Message

Ivan Khoronzhuk Dec. 6, 2017, 2:41 p.m. UTC
If rate is the same as set it's correct case.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
Based on net-next/master

 drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

David Miller Dec. 6, 2017, 9:35 p.m. UTC | #1
From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Date: Wed,  6 Dec 2017 16:41:18 +0200

> If rate is the same as set it's correct case.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
> Based on net-next/master
> 
>  drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> index e4d6edf..dbe9167 100644
> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> @@ -841,7 +841,7 @@ int cpdma_chan_set_rate(struct cpdma_chan *ch, u32 rate)
>  		return -EINVAL;
>  
>  	if (ch->rate == rate)
> -		return rate;
> +		return 0;

Looking at the one and only caller of this function, cpsw_ndo_set_tx_maxrate, it
makes sure this can never, ever, happen.

So I would instead remove this check completely since it can never trigger.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ivan Khoronzhuk Dec. 7, 2017, 7:48 p.m. UTC | #2
On Wed, Dec 06, 2017 at 04:35:45PM -0500, David Miller wrote:
> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> Date: Wed,  6 Dec 2017 16:41:18 +0200
> 
> > If rate is the same as set it's correct case.
> > 
> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> > ---
> > Based on net-next/master
> > 
> >  drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> > index e4d6edf..dbe9167 100644
> > --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> > +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> > @@ -841,7 +841,7 @@ int cpdma_chan_set_rate(struct cpdma_chan *ch, u32 rate)
> >  		return -EINVAL;
> >  
> >  	if (ch->rate == rate)
> > -		return rate;
> > +		return 0;
> 
> Looking at the one and only caller of this function, cpsw_ndo_set_tx_maxrate, it
> makes sure this can never, ever, happen.
In current circumstances yes, it will never happen.
But I caught it while adding related code and better return 0 if upper caller
doesn't have such check. Suppose that cpdma module is responsible for itself
and if it's critical I can send this patch along with whole related series.

> 
> So I would instead remove this check completely since it can never trigger.
David Miller Dec. 7, 2017, 7:50 p.m. UTC | #3
From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Date: Thu, 7 Dec 2017 21:48:56 +0200

> On Wed, Dec 06, 2017 at 04:35:45PM -0500, David Miller wrote:
>> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> Date: Wed,  6 Dec 2017 16:41:18 +0200
>> 
>> > If rate is the same as set it's correct case.
>> > 
>> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> > ---
>> > Based on net-next/master
>> > 
>> >  drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
>> > index e4d6edf..dbe9167 100644
>> > --- a/drivers/net/ethernet/ti/davinci_cpdma.c
>> > +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
>> > @@ -841,7 +841,7 @@ int cpdma_chan_set_rate(struct cpdma_chan *ch, u32 rate)
>> >  		return -EINVAL;
>> >  
>> >  	if (ch->rate == rate)
>> > -		return rate;
>> > +		return 0;
>> 
>> Looking at the one and only caller of this function, cpsw_ndo_set_tx_maxrate, it
>> makes sure this can never, ever, happen.
> In current circumstances yes, it will never happen.
> But I caught it while adding related code and better return 0 if upper caller
> doesn't have such check. Suppose that cpdma module is responsible for itself
> and if it's critical I can send this patch along with whole related series.

You have to decide one way or the other, who is responsible.

I think checking higher up is better because it's cheaper at that point to
look at the per-netdev queue rate setting before moving down deeper into the
driver specific data-structures.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ivan Khoronzhuk Dec. 7, 2017, 8:10 p.m. UTC | #4
On Thu, Dec 07, 2017 at 02:50:24PM -0500, David Miller wrote:
> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> Date: Thu, 7 Dec 2017 21:48:56 +0200
> 
> > On Wed, Dec 06, 2017 at 04:35:45PM -0500, David Miller wrote:
> >> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> Date: Wed,  6 Dec 2017 16:41:18 +0200
> >> 
> >> > If rate is the same as set it's correct case.
> >> > 
> >> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> > ---
> >> > Based on net-next/master
> >> > 
> >> >  drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> > 
> >> > diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> >> > index e4d6edf..dbe9167 100644
> >> > --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> >> > +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> >> > @@ -841,7 +841,7 @@ int cpdma_chan_set_rate(struct cpdma_chan *ch, u32 rate)
> >> >  		return -EINVAL;
> >> >  
> >> >  	if (ch->rate == rate)
> >> > -		return rate;
> >> > +		return 0;
> >> 
> >> Looking at the one and only caller of this function, cpsw_ndo_set_tx_maxrate, it
> >> makes sure this can never, ever, happen.
> > In current circumstances yes, it will never happen.
> > But I caught it while adding related code and better return 0 if upper caller
> > doesn't have such check. Suppose that cpdma module is responsible for itself
> > and if it's critical I can send this patch along with whole related series.
> 
> You have to decide one way or the other, who is responsible.
> 
> I think checking higher up is better because it's cheaper at that point to
> look at the per-netdev queue rate setting before moving down deeper into the
> driver specific data-structures.

No objection, but upper caller not always knows current rate and for doing like
this it needs read it first, and this is also some redundancy.
David Miller Dec. 7, 2017, 8:13 p.m. UTC | #5
From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Date: Thu, 7 Dec 2017 22:10:06 +0200

> On Thu, Dec 07, 2017 at 02:50:24PM -0500, David Miller wrote:
>> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> Date: Thu, 7 Dec 2017 21:48:56 +0200
>> 
>> > On Wed, Dec 06, 2017 at 04:35:45PM -0500, David Miller wrote:
>> >> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> >> Date: Wed,  6 Dec 2017 16:41:18 +0200
>> >> 
>> >> > If rate is the same as set it's correct case.
>> >> > 
>> >> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> >> > ---
>> >> > Based on net-next/master
>> >> > 
>> >> >  drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
>> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> > 
>> >> > diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
>> >> > index e4d6edf..dbe9167 100644
>> >> > --- a/drivers/net/ethernet/ti/davinci_cpdma.c
>> >> > +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
>> >> > @@ -841,7 +841,7 @@ int cpdma_chan_set_rate(struct cpdma_chan *ch, u32 rate)
>> >> >  		return -EINVAL;
>> >> >  
>> >> >  	if (ch->rate == rate)
>> >> > -		return rate;
>> >> > +		return 0;
>> >> 
>> >> Looking at the one and only caller of this function, cpsw_ndo_set_tx_maxrate, it
>> >> makes sure this can never, ever, happen.
>> > In current circumstances yes, it will never happen.
>> > But I caught it while adding related code and better return 0 if upper caller
>> > doesn't have such check. Suppose that cpdma module is responsible for itself
>> > and if it's critical I can send this patch along with whole related series.
>> 
>> You have to decide one way or the other, who is responsible.
>> 
>> I think checking higher up is better because it's cheaper at that point to
>> look at the per-netdev queue rate setting before moving down deeper into the
>> driver specific data-structures.
> 
> No objection, but upper caller not always knows current rate and for doing like
> this it needs read it first, and this is also some redundancy.

How can the upper caller not know the current rate?  The rate is
always stored in the generic netdev per-queue datastructure.

And that's what existing code checks right now.

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ivan Khoronzhuk Dec. 7, 2017, 8:21 p.m. UTC | #6
On Thu, Dec 07, 2017 at 03:13:15PM -0500, David Miller wrote:
> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> Date: Thu, 7 Dec 2017 22:10:06 +0200
> 
> > On Thu, Dec 07, 2017 at 02:50:24PM -0500, David Miller wrote:
> >> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> Date: Thu, 7 Dec 2017 21:48:56 +0200
> >> 
> >> > On Wed, Dec 06, 2017 at 04:35:45PM -0500, David Miller wrote:
> >> >> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> >> Date: Wed,  6 Dec 2017 16:41:18 +0200
> >> >> 
> >> >> > If rate is the same as set it's correct case.
> >> >> > 
> >> >> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> >> > ---
> >> >> > Based on net-next/master
> >> >> > 
> >> >> >  drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
> >> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >> > 
> >> >> > diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> >> >> > index e4d6edf..dbe9167 100644
> >> >> > --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> >> >> > +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> >> >> > @@ -841,7 +841,7 @@ int cpdma_chan_set_rate(struct cpdma_chan *ch, u32 rate)
> >> >> >  		return -EINVAL;
> >> >> >  
> >> >> >  	if (ch->rate == rate)
> >> >> > -		return rate;
> >> >> > +		return 0;
> >> >> 
> >> >> Looking at the one and only caller of this function, cpsw_ndo_set_tx_maxrate, it
> >> >> makes sure this can never, ever, happen.
> >> > In current circumstances yes, it will never happen.
> >> > But I caught it while adding related code and better return 0 if upper caller
> >> > doesn't have such check. Suppose that cpdma module is responsible for itself
> >> > and if it's critical I can send this patch along with whole related series.
> >> 
> >> You have to decide one way or the other, who is responsible.
> >> 
> >> I think checking higher up is better because it's cheaper at that point to
> >> look at the per-netdev queue rate setting before moving down deeper into the
> >> driver specific data-structures.
> > 
> > No objection, but upper caller not always knows current rate and for doing like
> > this it needs read it first, and this is also some redundancy.
> 
> How can the upper caller not know the current rate?  The rate is
> always stored in the generic netdev per-queue datastructure.
> 
> And that's what existing code checks right now.
Right now, when generic netdev only caller - yes.

Patch
diff mbox

diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index e4d6edf..dbe9167 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -841,7 +841,7 @@  int cpdma_chan_set_rate(struct cpdma_chan *ch, u32 rate)
 		return -EINVAL;
 
 	if (ch->rate == rate)
-		return rate;
+		return 0;
 
 	ctlr = ch->ctlr;
 	spin_lock_irqsave(&ctlr->lock, flags);