| Message ID | 20220517163450.240299-10-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ieee802154: Synchronous Tx support | expand |
Hi, On Tue, May 17, 2022 at 12:35 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > This is the slow path, we need to wait for each command to be processed > before continuing so let's introduce an helper which does the > transmission and blocks until it gets notified of its asynchronous > completion. This helper is going to be used when introducing scan > support. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > net/mac802154/ieee802154_i.h | 1 + > net/mac802154/tx.c | 46 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 47 insertions(+) > > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h > index a057827fc48a..b42c6ac789f5 100644 > --- a/net/mac802154/ieee802154_i.h > +++ b/net/mac802154/ieee802154_i.h > @@ -125,6 +125,7 @@ extern struct ieee802154_mlme_ops mac802154_mlme_wpan; > void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb); > void ieee802154_xmit_sync_worker(struct work_struct *work); > int ieee802154_sync_and_hold_queue(struct ieee802154_local *local); > +int ieee802154_mlme_tx_one(struct ieee802154_local *local, struct sk_buff *skb); > netdev_tx_t > ieee802154_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev); > netdev_tx_t > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c > index 38f74b8b6740..6cc4e5c7ba94 100644 > --- a/net/mac802154/tx.c > +++ b/net/mac802154/tx.c > @@ -128,6 +128,52 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local) > return ieee802154_sync_queue(local); > } > > +static int ieee802154_mlme_op_pre(struct ieee802154_local *local) > +{ > + return ieee802154_sync_and_hold_queue(local); > +} > + > +static int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *skb) > +{ > + int ret; > + > + /* Avoid possible calls to ->ndo_stop() when we asynchronously perform > + * MLME transmissions. > + */ > + rtnl_lock(); > + > + /* Ensure the device was not stopped, otherwise error out */ > + if (!local->open_count) > + return -EBUSY; > + No -EBUSY here, use ?-ENETDOWN?. You forgot rtnl_unlock() here. - Alex
Hi Alexander, aahringo@redhat.com wrote on Tue, 17 May 2022 20:41:41 -0400: > Hi, > > On Tue, May 17, 2022 at 12:35 PM Miquel Raynal > <miquel.raynal@bootlin.com> wrote: > > > > This is the slow path, we need to wait for each command to be processed > > before continuing so let's introduce an helper which does the > > transmission and blocks until it gets notified of its asynchronous > > completion. This helper is going to be used when introducing scan > > support. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > net/mac802154/ieee802154_i.h | 1 + > > net/mac802154/tx.c | 46 ++++++++++++++++++++++++++++++++++++ > > 2 files changed, 47 insertions(+) > > > > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h > > index a057827fc48a..b42c6ac789f5 100644 > > --- a/net/mac802154/ieee802154_i.h > > +++ b/net/mac802154/ieee802154_i.h > > @@ -125,6 +125,7 @@ extern struct ieee802154_mlme_ops mac802154_mlme_wpan; > > void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb); > > void ieee802154_xmit_sync_worker(struct work_struct *work); > > int ieee802154_sync_and_hold_queue(struct ieee802154_local *local); > > +int ieee802154_mlme_tx_one(struct ieee802154_local *local, struct sk_buff *skb); > > netdev_tx_t > > ieee802154_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev); > > netdev_tx_t > > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c > > index 38f74b8b6740..6cc4e5c7ba94 100644 > > --- a/net/mac802154/tx.c > > +++ b/net/mac802154/tx.c > > @@ -128,6 +128,52 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local) > > return ieee802154_sync_queue(local); > > } > > > > +static int ieee802154_mlme_op_pre(struct ieee802154_local *local) > > +{ > > + return ieee802154_sync_and_hold_queue(local); > > +} > > + > > +static int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *skb) > > +{ > > + int ret; > > + > > + /* Avoid possible calls to ->ndo_stop() when we asynchronously perform > > + * MLME transmissions. > > + */ > > + rtnl_lock(); > > + > > + /* Ensure the device was not stopped, otherwise error out */ > > + if (!local->open_count) > > + return -EBUSY; > > + > > No -EBUSY here, use ?-ENETDOWN?. Isn't it strange to return "Network is down" while we try to stop the device but fail to do so because, actually, it is still being used? > You forgot rtnl_unlock() here. I've blindly added those rtnl calls, I need to go through all of them once again and ensure all the error path release the lock. Thanks, Miquèl
Hi, On Wed, May 18, 2022 at 4:44 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Tue, 17 May 2022 20:41:41 -0400: > > > Hi, > > > > On Tue, May 17, 2022 at 12:35 PM Miquel Raynal > > <miquel.raynal@bootlin.com> wrote: > > > > > > This is the slow path, we need to wait for each command to be processed > > > before continuing so let's introduce an helper which does the > > > transmission and blocks until it gets notified of its asynchronous > > > completion. This helper is going to be used when introducing scan > > > support. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > net/mac802154/ieee802154_i.h | 1 + > > > net/mac802154/tx.c | 46 ++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 47 insertions(+) > > > > > > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h > > > index a057827fc48a..b42c6ac789f5 100644 > > > --- a/net/mac802154/ieee802154_i.h > > > +++ b/net/mac802154/ieee802154_i.h > > > @@ -125,6 +125,7 @@ extern struct ieee802154_mlme_ops mac802154_mlme_wpan; > > > void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb); > > > void ieee802154_xmit_sync_worker(struct work_struct *work); > > > int ieee802154_sync_and_hold_queue(struct ieee802154_local *local); > > > +int ieee802154_mlme_tx_one(struct ieee802154_local *local, struct sk_buff *skb); > > > netdev_tx_t > > > ieee802154_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev); > > > netdev_tx_t > > > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c > > > index 38f74b8b6740..6cc4e5c7ba94 100644 > > > --- a/net/mac802154/tx.c > > > +++ b/net/mac802154/tx.c > > > @@ -128,6 +128,52 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local) > > > return ieee802154_sync_queue(local); > > > } > > > > > > +static int ieee802154_mlme_op_pre(struct ieee802154_local *local) > > > +{ > > > + return ieee802154_sync_and_hold_queue(local); > > > +} > > > + > > > +static int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *skb) > > > +{ > > > + int ret; > > > + > > > + /* Avoid possible calls to ->ndo_stop() when we asynchronously perform > > > + * MLME transmissions. > > > + */ > > > + rtnl_lock(); > > > + > > > + /* Ensure the device was not stopped, otherwise error out */ > > > + if (!local->open_count) > > > + return -EBUSY; > > > + > > > > No -EBUSY here, use ?-ENETDOWN?. > > Isn't it strange to return "Network is down" while we try to stop the > device but fail to do so because, actually, it is still being used? > you are right. Maybe -EPERM, in a sense of whether the netdev state allows it or not. - Alex
Hi, On Wed, May 18, 2022 at 7:59 AM Alexander Aring <alex.aring@gmail.com> wrote: > > Hi, > > On Wed, May 18, 2022 at 4:44 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > aahringo@redhat.com wrote on Tue, 17 May 2022 20:41:41 -0400: > > > > > Hi, > > > > > > On Tue, May 17, 2022 at 12:35 PM Miquel Raynal > > > <miquel.raynal@bootlin.com> wrote: > > > > > > > > This is the slow path, we need to wait for each command to be processed > > > > before continuing so let's introduce an helper which does the > > > > transmission and blocks until it gets notified of its asynchronous > > > > completion. This helper is going to be used when introducing scan > > > > support. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > net/mac802154/ieee802154_i.h | 1 + > > > > net/mac802154/tx.c | 46 ++++++++++++++++++++++++++++++++++++ > > > > 2 files changed, 47 insertions(+) > > > > > > > > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h > > > > index a057827fc48a..b42c6ac789f5 100644 > > > > --- a/net/mac802154/ieee802154_i.h > > > > +++ b/net/mac802154/ieee802154_i.h > > > > @@ -125,6 +125,7 @@ extern struct ieee802154_mlme_ops mac802154_mlme_wpan; > > > > void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb); > > > > void ieee802154_xmit_sync_worker(struct work_struct *work); > > > > int ieee802154_sync_and_hold_queue(struct ieee802154_local *local); > > > > +int ieee802154_mlme_tx_one(struct ieee802154_local *local, struct sk_buff *skb); > > > > netdev_tx_t > > > > ieee802154_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev); > > > > netdev_tx_t > > > > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c > > > > index 38f74b8b6740..6cc4e5c7ba94 100644 > > > > --- a/net/mac802154/tx.c > > > > +++ b/net/mac802154/tx.c > > > > @@ -128,6 +128,52 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local) > > > > return ieee802154_sync_queue(local); > > > > } > > > > > > > > +static int ieee802154_mlme_op_pre(struct ieee802154_local *local) > > > > +{ > > > > + return ieee802154_sync_and_hold_queue(local); > > > > +} > > > > + > > > > +static int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *skb) > > > > +{ > > > > + int ret; > > > > + > > > > + /* Avoid possible calls to ->ndo_stop() when we asynchronously perform > > > > + * MLME transmissions. > > > > + */ > > > > + rtnl_lock(); > > > > + > > > > + /* Ensure the device was not stopped, otherwise error out */ > > > > + if (!local->open_count) > > > > + return -EBUSY; > > > > + > > > > > > No -EBUSY here, use ?-ENETDOWN?. > > > > Isn't it strange to return "Network is down" while we try to stop the > > device but fail to do so because, actually, it is still being used? > > > > you are right. Maybe -EPERM, in a sense of whether the netdev state > allows it or not. or maybe not, if this is the error the user gets by running iwpan. The problem I have with -EBUSY is that it indicates some resource is being used and will be solved at some time. Especially in a transmit function e.g. framebuffer. - Alex
alex.aring@gmail.com wrote on Wed, 18 May 2022 07:59:37 -0400: > Hi, > > On Wed, May 18, 2022 at 4:44 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > aahringo@redhat.com wrote on Tue, 17 May 2022 20:41:41 -0400: > > > > > Hi, > > > > > > On Tue, May 17, 2022 at 12:35 PM Miquel Raynal > > > <miquel.raynal@bootlin.com> wrote: > > > > > > > > This is the slow path, we need to wait for each command to be processed > > > > before continuing so let's introduce an helper which does the > > > > transmission and blocks until it gets notified of its asynchronous > > > > completion. This helper is going to be used when introducing scan > > > > support. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > net/mac802154/ieee802154_i.h | 1 + > > > > net/mac802154/tx.c | 46 ++++++++++++++++++++++++++++++++++++ > > > > 2 files changed, 47 insertions(+) > > > > > > > > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h > > > > index a057827fc48a..b42c6ac789f5 100644 > > > > --- a/net/mac802154/ieee802154_i.h > > > > +++ b/net/mac802154/ieee802154_i.h > > > > @@ -125,6 +125,7 @@ extern struct ieee802154_mlme_ops mac802154_mlme_wpan; > > > > void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb); > > > > void ieee802154_xmit_sync_worker(struct work_struct *work); > > > > int ieee802154_sync_and_hold_queue(struct ieee802154_local *local); > > > > +int ieee802154_mlme_tx_one(struct ieee802154_local *local, struct sk_buff *skb); > > > > netdev_tx_t > > > > ieee802154_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev); > > > > netdev_tx_t > > > > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c > > > > index 38f74b8b6740..6cc4e5c7ba94 100644 > > > > --- a/net/mac802154/tx.c > > > > +++ b/net/mac802154/tx.c > > > > @@ -128,6 +128,52 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local) > > > > return ieee802154_sync_queue(local); > > > > } > > > > > > > > +static int ieee802154_mlme_op_pre(struct ieee802154_local *local) > > > > +{ > > > > + return ieee802154_sync_and_hold_queue(local); > > > > +} > > > > + > > > > +static int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *skb) > > > > +{ > > > > + int ret; > > > > + > > > > + /* Avoid possible calls to ->ndo_stop() when we asynchronously perform > > > > + * MLME transmissions. > > > > + */ > > > > + rtnl_lock(); > > > > + > > > > + /* Ensure the device was not stopped, otherwise error out */ > > > > + if (!local->open_count) > > > > + return -EBUSY; > > > > + > > > > > > No -EBUSY here, use ?-ENETDOWN?. > > > > Isn't it strange to return "Network is down" while we try to stop the > > device but fail to do so because, actually, it is still being used? > > > > you are right. Maybe -EPERM, in a sense of whether the netdev state > allows it or not. Actually you were right in your fist review, "!open_count" means that the net iface is down, so returning -ENETDOWN is fine, I believe. Thanks, Miquèl
Hi, On Wed, May 18, 2022 at 8:44 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > alex.aring@gmail.com wrote on Wed, 18 May 2022 07:59:37 -0400: > > > Hi, > > > > On Wed, May 18, 2022 at 4:44 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Hi Alexander, > > > > > > aahringo@redhat.com wrote on Tue, 17 May 2022 20:41:41 -0400: > > > > > > > Hi, > > > > > > > > On Tue, May 17, 2022 at 12:35 PM Miquel Raynal > > > > <miquel.raynal@bootlin.com> wrote: > > > > > > > > > > This is the slow path, we need to wait for each command to be processed > > > > > before continuing so let's introduce an helper which does the > > > > > transmission and blocks until it gets notified of its asynchronous > > > > > completion. This helper is going to be used when introducing scan > > > > > support. > > > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > > --- > > > > > net/mac802154/ieee802154_i.h | 1 + > > > > > net/mac802154/tx.c | 46 ++++++++++++++++++++++++++++++++++++ > > > > > 2 files changed, 47 insertions(+) > > > > > > > > > > diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h > > > > > index a057827fc48a..b42c6ac789f5 100644 > > > > > --- a/net/mac802154/ieee802154_i.h > > > > > +++ b/net/mac802154/ieee802154_i.h > > > > > @@ -125,6 +125,7 @@ extern struct ieee802154_mlme_ops mac802154_mlme_wpan; > > > > > void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb); > > > > > void ieee802154_xmit_sync_worker(struct work_struct *work); > > > > > int ieee802154_sync_and_hold_queue(struct ieee802154_local *local); > > > > > +int ieee802154_mlme_tx_one(struct ieee802154_local *local, struct sk_buff *skb); > > > > > netdev_tx_t > > > > > ieee802154_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev); > > > > > netdev_tx_t > > > > > diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c > > > > > index 38f74b8b6740..6cc4e5c7ba94 100644 > > > > > --- a/net/mac802154/tx.c > > > > > +++ b/net/mac802154/tx.c > > > > > @@ -128,6 +128,52 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local) > > > > > return ieee802154_sync_queue(local); > > > > > } > > > > > > > > > > +static int ieee802154_mlme_op_pre(struct ieee802154_local *local) > > > > > +{ > > > > > + return ieee802154_sync_and_hold_queue(local); > > > > > +} > > > > > + > > > > > +static int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *skb) > > > > > +{ > > > > > + int ret; > > > > > + > > > > > + /* Avoid possible calls to ->ndo_stop() when we asynchronously perform > > > > > + * MLME transmissions. > > > > > + */ > > > > > + rtnl_lock(); > > > > > + > > > > > + /* Ensure the device was not stopped, otherwise error out */ > > > > > + if (!local->open_count) > > > > > + return -EBUSY; > > > > > + > > > > > > > > No -EBUSY here, use ?-ENETDOWN?. > > > > > > Isn't it strange to return "Network is down" while we try to stop the > > > device but fail to do so because, actually, it is still being used? > > > > > > > you are right. Maybe -EPERM, in a sense of whether the netdev state > > allows it or not. > > Actually you were right in your fist review, "!open_count" means > that the net iface is down, so returning -ENETDOWN is fine, I believe. > ah yes, you confused me! - Alex
diff --git a/net/mac802154/ieee802154_i.h b/net/mac802154/ieee802154_i.h index a057827fc48a..b42c6ac789f5 100644 --- a/net/mac802154/ieee802154_i.h +++ b/net/mac802154/ieee802154_i.h @@ -125,6 +125,7 @@ extern struct ieee802154_mlme_ops mac802154_mlme_wpan; void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb); void ieee802154_xmit_sync_worker(struct work_struct *work); int ieee802154_sync_and_hold_queue(struct ieee802154_local *local); +int ieee802154_mlme_tx_one(struct ieee802154_local *local, struct sk_buff *skb); netdev_tx_t ieee802154_monitor_start_xmit(struct sk_buff *skb, struct net_device *dev); netdev_tx_t diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c index 38f74b8b6740..6cc4e5c7ba94 100644 --- a/net/mac802154/tx.c +++ b/net/mac802154/tx.c @@ -128,6 +128,52 @@ int ieee802154_sync_and_hold_queue(struct ieee802154_local *local) return ieee802154_sync_queue(local); } +static int ieee802154_mlme_op_pre(struct ieee802154_local *local) +{ + return ieee802154_sync_and_hold_queue(local); +} + +static int ieee802154_mlme_tx(struct ieee802154_local *local, struct sk_buff *skb) +{ + int ret; + + /* Avoid possible calls to ->ndo_stop() when we asynchronously perform + * MLME transmissions. + */ + rtnl_lock(); + + /* Ensure the device was not stopped, otherwise error out */ + if (!local->open_count) + return -EBUSY; + + ieee802154_tx(local, skb); + ret = ieee802154_sync_queue(local); + + rtnl_unlock(); + + return ret; +} + +static void ieee802154_mlme_op_post(struct ieee802154_local *local) +{ + ieee802154_release_queue(local); +} + +int ieee802154_mlme_tx_one(struct ieee802154_local *local, struct sk_buff *skb) +{ + int ret; + + ret = ieee802154_mlme_op_pre(local); + if (ret) + return ret; + + ret = ieee802154_mlme_tx(local, skb); + + ieee802154_mlme_op_post(local); + + return ret; +} + static netdev_tx_t ieee802154_hot_tx(struct ieee802154_local *local, struct sk_buff *skb) {
This is the slow path, we need to wait for each command to be processed before continuing so let's introduce an helper which does the transmission and blocks until it gets notified of its asynchronous completion. This helper is going to be used when introducing scan support. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- net/mac802154/ieee802154_i.h | 1 + net/mac802154/tx.c | 46 ++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+)