| Message ID | 20210225215155.30509-4-dariobin@libero.it (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: c_can: add support to 64 message objects | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
On 25.02.2021 22:51:52, Dario Binacchi wrote: > According to commit 640916db2bf7 ("can: c_can: Make it SMP safe") let RX use > IF1 (i.e. IF_RX) and TX use IF2 (i.e. IF_TX). Is this a fix? Marc > > Signed-off-by: Dario Binacchi <dariobin@libero.it> > --- > > (no changes since v1) > > drivers/net/can/c_can/c_can.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c > index dbcc1c1c92d6..69526c3a671c 100644 > --- a/drivers/net/can/c_can/c_can.c > +++ b/drivers/net/can/c_can/c_can.c > @@ -732,7 +732,7 @@ static void c_can_do_tx(struct net_device *dev) > idx--; > pend &= ~(1 << idx); > obj = idx + C_CAN_MSG_OBJ_TX_FIRST; > - c_can_inval_tx_object(dev, IF_RX, obj); > + c_can_inval_tx_object(dev, IF_TX, obj); > can_get_echo_skb(dev, idx, NULL); > bytes += priv->dlc[idx]; > pkts++; > -- > 2.17.1 > >
Hi Marc, > Il 26/02/2021 09:44 Marc Kleine-Budde <mkl@pengutronix.de> ha scritto: > > > On 25.02.2021 22:51:52, Dario Binacchi wrote: > > According to commit 640916db2bf7 ("can: c_can: Make it SMP safe") let RX use > > IF1 (i.e. IF_RX) and TX use IF2 (i.e. IF_TX). > > Is this a fix? > I think that If I consider what is described in the 640916db2bf7 commit, using the IF_RX interface in a tx routine is wrong. Thanks and regards Dario > Marc > > > > > Signed-off-by: Dario Binacchi <dariobin@libero.it> > > --- > > > > (no changes since v1) > > > > drivers/net/can/c_can/c_can.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c > > index dbcc1c1c92d6..69526c3a671c 100644 > > --- a/drivers/net/can/c_can/c_can.c > > +++ b/drivers/net/can/c_can/c_can.c > > @@ -732,7 +732,7 @@ static void c_can_do_tx(struct net_device *dev) > > idx--; > > pend &= ~(1 << idx); > > obj = idx + C_CAN_MSG_OBJ_TX_FIRST; > > - c_can_inval_tx_object(dev, IF_RX, obj); > > + c_can_inval_tx_object(dev, IF_TX, obj); > > can_get_echo_skb(dev, idx, NULL); > > bytes += priv->dlc[idx]; > > pkts++; > > -- > > 2.17.1 > > > > > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung West/Dortmund | Phone: +49-231-2826-924 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 28.02.2021 11:35:31, Dario Binacchi wrote: > > On 25.02.2021 22:51:52, Dario Binacchi wrote: > > > According to commit 640916db2bf7 ("can: c_can: Make it SMP safe") let RX use > > > IF1 (i.e. IF_RX) and TX use IF2 (i.e. IF_TX). > > > > Is this a fix? > > > > I think that If I consider what is described in the 640916db2bf7 > commit, using the IF_RX interface in a tx routine is wrong. Yes, IF_RX is used in c_can_do_tx(), but that's called from c_can_poll(), which runs ins NAPI. As far as I understand 640916db2bf7 ("can: c_can: Make it SMP safe") fixes the race condition that c_can_poll() and c_can_start_xmit() both access the same IF. See again the patch description: | The hardware has two message control interfaces, but the code only uses the | first one. So on SMP the following can be observed: | | CPU0 CPU1 | rx_poll() | write IF1 xmit() | write IF1 | write IF1 It's not 100% accurate, as the race condition is not just c_can_do_rx_poll() against the c_can_start_xmit(), but the whole c_can_poll() against c_can_start_xmit(). If you think my analysis is correct, please update the patch and add a comment to clarify why IF_RX is used instead of changing it to IF_TX. regards, Marc
> Il 01/03/2021 12:36 Marc Kleine-Budde <mkl@pengutronix.de> ha scritto: > > > On 28.02.2021 11:35:31, Dario Binacchi wrote: > > > On 25.02.2021 22:51:52, Dario Binacchi wrote: > > > > According to commit 640916db2bf7 ("can: c_can: Make it SMP safe") let RX use > > > > IF1 (i.e. IF_RX) and TX use IF2 (i.e. IF_TX). > > > > > > Is this a fix? > > > > > > > I think that If I consider what is described in the 640916db2bf7 > > commit, using the IF_RX interface in a tx routine is wrong. > > Yes, IF_RX is used in c_can_do_tx(), but that's called from > c_can_poll(), which runs ins NAPI. Yes, you are right. I was misled by the name of the function. > > As far as I understand 640916db2bf7 ("can: c_can: Make it SMP safe") > fixes the race condition that c_can_poll() and c_can_start_xmit() both > access the same IF. See again the patch description: > > | The hardware has two message control interfaces, but the code only uses the > | first one. So on SMP the following can be observed: > | > | CPU0 CPU1 > | rx_poll() > | write IF1 xmit() > | write IF1 > | write IF1 > > It's not 100% accurate, as the race condition is not just > c_can_do_rx_poll() against the c_can_start_xmit(), but the whole > c_can_poll() against c_can_start_xmit(). > > If you think my analysis is correct, please update the patch and add a > comment to clarify why IF_RX is used instead of changing it to IF_TX. I agree with you, I'll do it. Thanks and regards, Dario > > regards, > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung West/Dortmund | Phone: +49-231-2826-924 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c index dbcc1c1c92d6..69526c3a671c 100644 --- a/drivers/net/can/c_can/c_can.c +++ b/drivers/net/can/c_can/c_can.c @@ -732,7 +732,7 @@ static void c_can_do_tx(struct net_device *dev) idx--; pend &= ~(1 << idx); obj = idx + C_CAN_MSG_OBJ_TX_FIRST; - c_can_inval_tx_object(dev, IF_RX, obj); + c_can_inval_tx_object(dev, IF_TX, obj); can_get_echo_skb(dev, idx, NULL); bytes += priv->dlc[idx]; pkts++;
According to commit 640916db2bf7 ("can: c_can: Make it SMP safe") let RX use IF1 (i.e. IF_RX) and TX use IF2 (i.e. IF_TX). Signed-off-by: Dario Binacchi <dariobin@libero.it> --- (no changes since v1) drivers/net/can/c_can/c_can.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)