diff mbox series

[net-next,17/18] net: mac802154: Let drivers provide their own beacons implementation

Message ID 20211222155743.256280-18-miquel.raynal@bootlin.com (mailing list archive)
State Superseded
Headers show
Series IEEE 802.15.4 passive scan support | expand

Commit Message

Miquel Raynal Dec. 22, 2021, 3:57 p.m. UTC
So far only a pure software procedure for sending beacons was possible.
Let's create a couple of driver's hooks in order to allow the device
drivers to provide their own implementation. If not provided, fallback
to the pure software logic.

It is possible for device drivers to only support a specific type of
request and return -EOPNOTSUPP otherwise, this will have the same effect
as not providing any hooks for these specific cases.

Co-developed-by: David Girault <david.girault@qorvo.com>
Signed-off-by: David Girault <david.girault@qorvo.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/net/mac802154.h    | 13 +++++++++++++
 net/mac802154/driver-ops.h | 33 +++++++++++++++++++++++++++++++++
 net/mac802154/scan.c       | 17 +++++++++++++++++
 net/mac802154/trace.h      | 21 +++++++++++++++++++++
 4 files changed, 84 insertions(+)

Comments

Alexander Aring Dec. 28, 2021, 10:25 p.m. UTC | #1
Hi,

On Wed, 22 Dec 2021 at 10:58, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> So far only a pure software procedure for sending beacons was possible.
> Let's create a couple of driver's hooks in order to allow the device
> drivers to provide their own implementation. If not provided, fallback
> to the pure software logic.
>

Can you name a SoftMAC transceiver which provides such an "offload" feature?

- Alex
David Girault Dec. 30, 2021, 4:59 p.m. UTC | #2
Hi Alexander,

At Qorvo, we have developped a SoftMAC driver for our DW3000 chip that will benefit such API.

To be short, beacon sending is controller by our driver to be synchronized chip clock or delayed until 
other operation in progress (a ranging for example).

Regards,
David Girault
Alexander Aring Dec. 30, 2021, 7:48 p.m. UTC | #3
Hi,

On Thu, 30 Dec 2021 at 12:00, David Girault <David.Girault@qorvo.com> wrote:
>
> Hi Alexander,
>
> At Qorvo, we have developped a SoftMAC driver for our DW3000 chip that will benefit such API.
>
Do you want to bring this driver upstream as well? Currently those
callbacks will be introduced but no user is there.

> To be short, beacon sending is controller by our driver to be synchronized chip clock or delayed until
> other operation in progress (a ranging for example).
>

ok.

- Alex
Miquel Raynal Jan. 5, 2022, 8:48 a.m. UTC | #4
Hi Alexander,

alex.aring@gmail.com wrote on Thu, 30 Dec 2021 14:48:41 -0500:

> Hi,
> 
> On Thu, 30 Dec 2021 at 12:00, David Girault <David.Girault@qorvo.com> wrote:
> >
> > Hi Alexander,
> >
> > At Qorvo, we have developped a SoftMAC driver for our DW3000 chip that will benefit such API.
> >  
> Do you want to bring this driver upstream as well? Currently those
> callbacks will be introduced but no user is there.

I think so far the upstream fate of the DW3000 driver has not been ruled
out so let's assume it won't be upstreamed (at least not fully), that's
also why we decided to begin with the hwsim driver.

However, when designing this series, it appeared quite clear that any
hardMAC driver would need this type of interface. The content of the
interface, I agree, could be further discussed and even edited, but the
main idea of giving the information to the phy driver about what is
happening regarding eg. scan operations or beacon frames, might make
sense regardless of the current users, no?

This being said, if other people decide to upstream a hardMAC driver
and need these hooks to behave a little bit differently, it's their
right to tweak them and that would also be part of the game.

Although we might not need these hooks in a near future at all if we
move to the filtering modes, because the promiscuous call with the
specific level might indicate to the device how it should configure
itself already.

> > To be short, beacon sending is controller by our driver to be synchronized chip clock or delayed until
> > other operation in progress (a ranging for example).
> >  
> 
> ok.
> 
> - Alex

Thanks,
Miquèl
Alexander Aring Jan. 6, 2022, 12:23 a.m. UTC | #5
Hi,

On Wed, 5 Jan 2022 at 03:48, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> alex.aring@gmail.com wrote on Thu, 30 Dec 2021 14:48:41 -0500:
>
> > Hi,
> >
> > On Thu, 30 Dec 2021 at 12:00, David Girault <David.Girault@qorvo.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > At Qorvo, we have developped a SoftMAC driver for our DW3000 chip that will benefit such API.
> > >
> > Do you want to bring this driver upstream as well? Currently those
> > callbacks will be introduced but no user is there.
>
> I think so far the upstream fate of the DW3000 driver has not been ruled
> out so let's assume it won't be upstreamed (at least not fully), that's
> also why we decided to begin with the hwsim driver.
>

ok.

> However, when designing this series, it appeared quite clear that any
> hardMAC driver would need this type of interface. The content of the
> interface, I agree, could be further discussed and even edited, but the
> main idea of giving the information to the phy driver about what is
> happening regarding eg. scan operations or beacon frames, might make
> sense regardless of the current users, no?
>

A HardMAC driver does not use this driver interface... but there
exists a SoftMAC driver for a HardMAC transceiver. This driver
currently works because we use dataframes only... It will not support
scanning currently and somehow we should make iit not available for
drivers like that and for drivers which don't set symbol duration.
They need to be fixed.

> This being said, if other people decide to upstream a hardMAC driver
> and need these hooks to behave a little bit differently, it's their
> right to tweak them and that would also be part of the game.
>
> Although we might not need these hooks in a near future at all if we
> move to the filtering modes, because the promiscuous call with the
> specific level might indicate to the device how it should configure
> itself already.
>

My concern is that somebody else might want to remove those callbacks
because they are not used.

- Alex
Miquel Raynal Jan. 6, 2022, 7:15 p.m. UTC | #6
Hi Alexander,

alex.aring@gmail.com wrote on Wed, 5 Jan 2022 19:23:04 -0500:

> Hi,
> 
> On Wed, 5 Jan 2022 at 03:48, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > alex.aring@gmail.com wrote on Thu, 30 Dec 2021 14:48:41 -0500:
> >  
> > > Hi,
> > >
> > > On Thu, 30 Dec 2021 at 12:00, David Girault <David.Girault@qorvo.com> wrote:  
> > > >
> > > > Hi Alexander,
> > > >
> > > > At Qorvo, we have developped a SoftMAC driver for our DW3000 chip that will benefit such API.
> > > >  
> > > Do you want to bring this driver upstream as well? Currently those
> > > callbacks will be introduced but no user is there.  
> >
> > I think so far the upstream fate of the DW3000 driver has not been ruled
> > out so let's assume it won't be upstreamed (at least not fully), that's
> > also why we decided to begin with the hwsim driver.
> >  
> 
> ok.
> 
> > However, when designing this series, it appeared quite clear that any
> > hardMAC driver would need this type of interface. The content of the
> > interface, I agree, could be further discussed and even edited, but the
> > main idea of giving the information to the phy driver about what is
> > happening regarding eg. scan operations or beacon frames, might make
> > sense regardless of the current users, no?
> >  
> 
> A HardMAC driver does not use this driver interface... but there
> exists a SoftMAC driver for a HardMAC transceiver. This driver
> currently works because we use dataframes only... It will not support
> scanning currently and somehow we should make iit not available for
> drivers like that and for drivers which don't set symbol duration.
> They need to be fixed.

My bad. I did not look at it correctly. I made a mistake when talking
about a hardMAC.

Instead, it is a "custom" low level MAC layer. I believe we can compare
the current mac802154 layer mostly to the MLME that is mentioned in the
spec. Well here the additional layer that needs these hooks would be
the MCPS. I don't know if this will be upstreamed or not, but the need
for these hooks is real if such an intermediate low level MAC layer
gets introduced.

In v2 I will get rid of the two patches adding "driver access" to scans
and beacons in order to facilitate the merge of the big part. Then we
will have plenty of time to discuss how we can create such an interface.
Perhaps I'll be able to propose more code as well to make use of these
hooks, we will see.

> > This being said, if other people decide to upstream a hardMAC driver
> > and need these hooks to behave a little bit differently, it's their
> > right to tweak them and that would also be part of the game.
> >
> > Although we might not need these hooks in a near future at all if we
> > move to the filtering modes, because the promiscuous call with the
> > specific level might indicate to the device how it should configure
> > itself already.
> >  
> 
> My concern is that somebody else might want to remove those callbacks
> because they are not used.

Yes, this is likely to happen quickly because of robots :)

Thanks,
Miquèl
Alexander Aring Jan. 7, 2022, 4:21 a.m. UTC | #7
Hi,

On Thu, 6 Jan 2022 at 14:15, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> alex.aring@gmail.com wrote on Wed, 5 Jan 2022 19:23:04 -0500:
>
...
> >
> > A HardMAC driver does not use this driver interface... but there
> > exists a SoftMAC driver for a HardMAC transceiver. This driver
> > currently works because we use dataframes only... It will not support
> > scanning currently and somehow we should make iit not available for
> > drivers like that and for drivers which don't set symbol duration.
> > They need to be fixed.
>
> My bad. I did not look at it correctly. I made a mistake when talking
> about a hardMAC.
>
> Instead, it is a "custom" low level MAC layer. I believe we can compare
> the current mac802154 layer mostly to the MLME that is mentioned in the
> spec. Well here the additional layer that needs these hooks would be
> the MCPS. I don't know if this will be upstreamed or not, but the need
> for these hooks is real if such an intermediate low level MAC layer
> gets introduced.
>
> In v2 I will get rid of the two patches adding "driver access" to scans
> and beacons in order to facilitate the merge of the big part. Then we
> will have plenty of time to discuss how we can create such an interface.
> Perhaps I'll be able to propose more code as well to make use of these
> hooks, we will see.
>

That the we have a standardised interface between Ieee802154 and
(HardMAC or SoftMAC(mac802154)) (see cfg802154_ops) which is defined
according the spec would make it more "stable" that it will work with
different HardMAC transceivers (which follows that interface) and
mac802154 stack (which also follows that interface). If I understood
you correctly.
I think this is one reason why we are not having any HardMAC
transceivers driver supported in a proper way yet.

I can also imagine about a hwsim HardMAC transceiver which redirects
cfg802154 to mac802154 SoftMAC instance again (something like that),
to have a virtual HardMAC transceiver for testing purpose, etc. In
theory that should work...

- Alex
Miquel Raynal Jan. 7, 2022, 7:40 a.m. UTC | #8
Hi Alexander,

alex.aring@gmail.com wrote on Thu, 6 Jan 2022 23:21:45 -0500:

> Hi,
> 
> On Thu, 6 Jan 2022 at 14:15, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Alexander,
> >
> > alex.aring@gmail.com wrote on Wed, 5 Jan 2022 19:23:04 -0500:
> >  
> ...
> > >
> > > A HardMAC driver does not use this driver interface... but there
> > > exists a SoftMAC driver for a HardMAC transceiver. This driver
> > > currently works because we use dataframes only... It will not support
> > > scanning currently and somehow we should make iit not available for
> > > drivers like that and for drivers which don't set symbol duration.
> > > They need to be fixed.  
> >
> > My bad. I did not look at it correctly. I made a mistake when talking
> > about a hardMAC.
> >
> > Instead, it is a "custom" low level MAC layer. I believe we can compare
> > the current mac802154 layer mostly to the MLME that is mentioned in the
> > spec. Well here the additional layer that needs these hooks would be
> > the MCPS. I don't know if this will be upstreamed or not, but the need
> > for these hooks is real if such an intermediate low level MAC layer
> > gets introduced.
> >
> > In v2 I will get rid of the two patches adding "driver access" to scans
> > and beacons in order to facilitate the merge of the big part. Then we
> > will have plenty of time to discuss how we can create such an interface.
> > Perhaps I'll be able to propose more code as well to make use of these
> > hooks, we will see.
> >  
> 
> That the we have a standardised interface between Ieee802154 and
> (HardMAC or SoftMAC(mac802154)) (see cfg802154_ops) which is defined
> according the spec would make it more "stable" that it will work with
> different HardMAC transceivers (which follows that interface) and
> mac802154 stack (which also follows that interface). If I understood
> you correctly.


I am not sure. I am really talking about a softMAC. I am not sure
where to put that layer "vertically" but according to the spec the MCPS
(MAC Common Part Sublayer) is the layer that contains the data
primitives, while MLME has been designed for management and
configuration.

> I think this is one reason why we are not having any HardMAC
> transceivers driver supported in a proper way yet.
> 
> I can also imagine about a hwsim HardMAC transceiver which redirects
> cfg802154 to mac802154 SoftMAC instance again (something like that),
> to have a virtual HardMAC transceiver for testing purpose, etc. In
> theory that should work...

Yeah I see what you mean, but IMHO that's basically duplicating the
softMAC layer, we already have hwsim wired to cfg802154 through
mac802154. In a certain way we could argue that this is a hardMAC =)

Thanks,
Miquèl
Alexander Aring Jan. 11, 2022, 1:43 p.m. UTC | #9
Hi,

On Fri, 7 Jan 2022 at 02:40, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Alexander,
>
> alex.aring@gmail.com wrote on Thu, 6 Jan 2022 23:21:45 -0500:
>
> > Hi,
> >
> > On Thu, 6 Jan 2022 at 14:15, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > Hi Alexander,
> > >
> > > alex.aring@gmail.com wrote on Wed, 5 Jan 2022 19:23:04 -0500:
> > >
> > ...
> > > >
> > > > A HardMAC driver does not use this driver interface... but there
> > > > exists a SoftMAC driver for a HardMAC transceiver. This driver
> > > > currently works because we use dataframes only... It will not support
> > > > scanning currently and somehow we should make iit not available for
> > > > drivers like that and for drivers which don't set symbol duration.
> > > > They need to be fixed.
> > >
> > > My bad. I did not look at it correctly. I made a mistake when talking
> > > about a hardMAC.
> > >
> > > Instead, it is a "custom" low level MAC layer. I believe we can compare
> > > the current mac802154 layer mostly to the MLME that is mentioned in the
> > > spec. Well here the additional layer that needs these hooks would be
> > > the MCPS. I don't know if this will be upstreamed or not, but the need
> > > for these hooks is real if such an intermediate low level MAC layer
> > > gets introduced.
> > >
> > > In v2 I will get rid of the two patches adding "driver access" to scans
> > > and beacons in order to facilitate the merge of the big part. Then we
> > > will have plenty of time to discuss how we can create such an interface.
> > > Perhaps I'll be able to propose more code as well to make use of these
> > > hooks, we will see.
> > >
> >
> > That the we have a standardised interface between Ieee802154 and
> > (HardMAC or SoftMAC(mac802154)) (see cfg802154_ops) which is defined
> > according the spec would make it more "stable" that it will work with
> > different HardMAC transceivers (which follows that interface) and
> > mac802154 stack (which also follows that interface). If I understood
> > you correctly.
>
>
> I am not sure. I am really talking about a softMAC. I am not sure
> where to put that layer "vertically" but according to the spec the MCPS
> (MAC Common Part Sublayer) is the layer that contains the data
> primitives, while MLME has been designed for management and
> configuration.
>

ok.

> > I think this is one reason why we are not having any HardMAC
> > transceivers driver supported in a proper way yet.
> >
> > I can also imagine about a hwsim HardMAC transceiver which redirects
> > cfg802154 to mac802154 SoftMAC instance again (something like that),
> > to have a virtual HardMAC transceiver for testing purpose, etc. In
> > theory that should work...
>
> Yeah I see what you mean, but IMHO that's basically duplicating the
> softMAC layer, we already have hwsim wired to cfg802154 through
> mac802154. In a certain way we could argue that this is a hardMAC =)

Would be good to show people "here is how to write a HardMAC
driver..." if this is even possible without any change yet.

- Alex
diff mbox series

Patch

diff --git a/include/net/mac802154.h b/include/net/mac802154.h
index 97aefba7bf96..72978fb72a3a 100644
--- a/include/net/mac802154.h
+++ b/include/net/mac802154.h
@@ -214,6 +214,16 @@  enum ieee802154_hw_flags {
  *	  Exits the scan mode and returns to a fully functioning state.
  *	  Should only be provided if ->enter_scan_mode() is populated.
  *	  Returns either zero, or negative errno.
+ *
+ * send_beacons
+ *	  Send beacons at a fixed rate over the current channel.
+ *	  Can be NULL, if the driver doesn't support sending beacons by itself.
+ *	  Returns either zero, or negative errno.
+ *
+ * stop_beacons
+ *	  Stops sending beacons.
+ *	  Should only be provided if ->send_beacons() is populated.
+ *	  Returns either zero, or negative errno.
  */
 struct ieee802154_ops {
 	struct module	*owner;
@@ -243,6 +253,9 @@  struct ieee802154_ops {
 	int		(*enter_scan_mode)(struct ieee802154_hw *hw,
 					   struct cfg802154_scan_request *request);
 	int		(*exit_scan_mode)(struct ieee802154_hw *hw);
+	int		(*send_beacons)(struct ieee802154_hw *hw,
+					struct cfg802154_beacons_request *request);
+	int		(*stop_beacons)(struct ieee802154_hw *hw);
 };
 
 /**
diff --git a/net/mac802154/driver-ops.h b/net/mac802154/driver-ops.h
index 2f5650f7bf91..003e6edee049 100644
--- a/net/mac802154/driver-ops.h
+++ b/net/mac802154/driver-ops.h
@@ -315,4 +315,37 @@  static inline int drv_exit_scan_mode(struct ieee802154_local *local)
 	return ret;
 }
 
+static inline int drv_send_beacons(struct ieee802154_local *local,
+				   struct cfg802154_beacons_request *request)
+{
+	int ret;
+
+	might_sleep();
+
+	if (!local->ops->send_beacons || !local->ops->stop_beacons)
+		return -EOPNOTSUPP;
+
+	trace_802154_drv_send_beacons(local, request);
+	ret = local->ops->send_beacons(&local->hw, request);
+	trace_802154_drv_return_int(local, ret);
+
+	return ret;
+}
+
+static inline int drv_stop_beacons(struct ieee802154_local *local)
+{
+	int ret;
+
+	might_sleep();
+
+	if (!local->ops->send_beacons || !local->ops->stop_beacons)
+		return -EOPNOTSUPP;
+
+	trace_802154_drv_stop_beacons(local);
+	ret = local->ops->stop_beacons(&local->hw);
+	trace_802154_drv_return_int(local, ret);
+
+	return ret;
+}
+
 #endif /* __MAC802154_DRIVER_OPS */
diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c
index b334fa856c00..55af9d16744a 100644
--- a/net/mac802154/scan.c
+++ b/net/mac802154/scan.c
@@ -109,6 +109,7 @@  int mac802154_send_beacons_locked(struct ieee802154_sub_if_data *sdata,
 {
 	struct ieee802154_local *local = sdata->local;
 	unsigned int interval;
+	int ret;
 
 	lockdep_assert_held(&local->beacons_lock);
 
@@ -117,6 +118,14 @@  int mac802154_send_beacons_locked(struct ieee802154_sub_if_data *sdata,
 
 	local->ongoing_beacons_request = true;
 
+	/* Either let the hardware handle the beacons or handle them manually */
+	ret = drv_send_beacons(local, request);
+	if (ret != -EOPNOTSUPP) {
+		if (ret)
+			local->ongoing_beacons_request = false;
+		return ret;
+	}
+
 	interval = mac802154_scan_get_channel_time(request->interval,
 						   request->wpan_phy->symbol_duration);
 
@@ -151,6 +160,8 @@  int mac802154_send_beacons_locked(struct ieee802154_sub_if_data *sdata,
 
 int mac802154_stop_beacons_locked(struct ieee802154_local *local)
 {
+	int ret;
+
 	lockdep_assert_held(&local->beacons_lock);
 
 	if (!local->ongoing_beacons_request)
@@ -159,6 +170,12 @@  int mac802154_stop_beacons_locked(struct ieee802154_local *local)
 	local->ongoing_beacons_request = false;
 	cancel_delayed_work(&local->beacons_work);
 
+	ret = drv_stop_beacons(local);
+	if (ret != -EOPNOTSUPP)
+		return ret;
+
+	cancel_delayed_work(&local->beacons_work);
+
 	return 0;
 }
 
diff --git a/net/mac802154/trace.h b/net/mac802154/trace.h
index 9c0a4f07ced1..b487523f83c3 100644
--- a/net/mac802154/trace.h
+++ b/net/mac802154/trace.h
@@ -292,6 +292,27 @@  DEFINE_EVENT(local_only_evt4, 802154_drv_exit_scan_mode,
 	TP_ARGS(local)
 );
 
+TRACE_EVENT(802154_drv_send_beacons,
+	TP_PROTO(struct ieee802154_local *local,
+		 struct cfg802154_beacons_request *request),
+	TP_ARGS(local, request),
+	TP_STRUCT__entry(
+		LOCAL_ENTRY
+		__field(u8, interval)
+	),
+	TP_fast_assign(
+		LOCAL_ASSIGN;
+		__entry->interval = request->interval;
+	),
+	TP_printk(LOCAL_PR_FMT ", send beacons at interval: %d",
+		  LOCAL_PR_ARG, __entry->interval)
+);
+
+DEFINE_EVENT(local_only_evt4, 802154_drv_stop_beacons,
+	TP_PROTO(struct ieee802154_local *local),
+	TP_ARGS(local)
+);
+
 #endif /* !__MAC802154_DRIVER_TRACE || TRACE_HEADER_MULTI_READ */
 
 #undef TRACE_INCLUDE_PATH