Re: [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case

[Ivan Khoronzhuk]

On Thu, Dec 07, 2017 at 03:13:15PM -0500, David Miller wrote:
> From: Ivan Khoronzhuk 
> 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 
> >> 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 
> >> >> 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 
> >> >> > ---
> >> >> > 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.

-- 
Regards,
Ivan Khoronzhuk

Re: [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case

[David Miller]

From: Ivan Khoronzhuk 
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 
>> 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 
>> >> 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 
>> >> > ---
>> >> > 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.

Re: [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case

[Ivan Khoronzhuk]

On Thu, Dec 07, 2017 at 02:50:24PM -0500, David Miller wrote:
> From: Ivan Khoronzhuk 
> 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 
> >> 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 
> >> > ---
> >> > 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.

-- 
Regards,
Ivan Khoronzhuk

Re: [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case

[David Miller]

From: Ivan Khoronzhuk 
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 
>> 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 
>> > ---
>> > 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.

Re: [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case

[Ivan Khoronzhuk]

On Wed, Dec 06, 2017 at 04:35:45PM -0500, David Miller wrote:
> From: Ivan Khoronzhuk 
> 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 
> > ---
> > 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.

-- 
Regards,
Ivan Khoronzhuk

Re: [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case

[David Miller]

From: Ivan Khoronzhuk 
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 
> ---
> 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.