* [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case @ 2017-12-06 14:41 Ivan Khoronzhuk 2017-12-06 21:35 ` David Miller 0 siblings, 1 reply; 7+ messages in thread From: Ivan Khoronzhuk @ 2017-12-06 14:41 UTC (permalink / raw) To: grygorii.strashko Cc: netdev, davem, linux-omap, linux-kernel, Ivan Khoronzhuk 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; ctlr = ch->ctlr; spin_lock_irqsave(&ctlr->lock, flags); -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case 2017-12-06 14:41 [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case Ivan Khoronzhuk @ 2017-12-06 21:35 ` David Miller 2017-12-07 19:48 ` Ivan Khoronzhuk 0 siblings, 1 reply; 7+ messages in thread From: David Miller @ 2017-12-06 21:35 UTC (permalink / raw) To: ivan.khoronzhuk; +Cc: grygorii.strashko, netdev, linux-omap, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case 2017-12-06 21:35 ` David Miller @ 2017-12-07 19:48 ` Ivan Khoronzhuk 2017-12-07 19:50 ` David Miller 0 siblings, 1 reply; 7+ messages in thread From: Ivan Khoronzhuk @ 2017-12-07 19:48 UTC (permalink / raw) To: David Miller; +Cc: grygorii.strashko, netdev, linux-omap, linux-kernel 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. -- Regards, Ivan Khoronzhuk ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case 2017-12-07 19:48 ` Ivan Khoronzhuk @ 2017-12-07 19:50 ` David Miller 2017-12-07 20:10 ` Ivan Khoronzhuk 0 siblings, 1 reply; 7+ messages in thread From: David Miller @ 2017-12-07 19:50 UTC (permalink / raw) To: ivan.khoronzhuk; +Cc: grygorii.strashko, netdev, linux-omap, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case 2017-12-07 19:50 ` David Miller @ 2017-12-07 20:10 ` Ivan Khoronzhuk 2017-12-07 20:13 ` David Miller 0 siblings, 1 reply; 7+ messages in thread From: Ivan Khoronzhuk @ 2017-12-07 20:10 UTC (permalink / raw) To: David Miller; +Cc: grygorii.strashko, netdev, linux-omap, linux-kernel 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. -- Regards, Ivan Khoronzhuk ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case 2017-12-07 20:10 ` Ivan Khoronzhuk @ 2017-12-07 20:13 ` David Miller 2017-12-07 20:21 ` Ivan Khoronzhuk 0 siblings, 1 reply; 7+ messages in thread From: David Miller @ 2017-12-07 20:13 UTC (permalink / raw) To: ivan.khoronzhuk; +Cc: grygorii.strashko, netdev, linux-omap, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case 2017-12-07 20:13 ` David Miller @ 2017-12-07 20:21 ` Ivan Khoronzhuk 0 siblings, 0 replies; 7+ messages in thread From: Ivan Khoronzhuk @ 2017-12-07 20:21 UTC (permalink / raw) To: David Miller; +Cc: grygorii.strashko, netdev, linux-omap, linux-kernel 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. -- Regards, Ivan Khoronzhuk ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-12-07 20:21 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-12-06 14:41 [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case Ivan Khoronzhuk 2017-12-06 21:35 ` David Miller 2017-12-07 19:48 ` Ivan Khoronzhuk 2017-12-07 19:50 ` David Miller 2017-12-07 20:10 ` Ivan Khoronzhuk 2017-12-07 20:13 ` David Miller 2017-12-07 20:21 ` Ivan Khoronzhuk
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).