* [PATCH] Print warnings for missing cfg80211_ops implementations
@ 2015-12-16 21:43 Ola Olsson
2015-12-17 1:16 ` Joe Perches
0 siblings, 1 reply; 8+ messages in thread
From: Ola Olsson @ 2015-12-16 21:43 UTC (permalink / raw)
To: johannes; +Cc: linux-wireless, Ola Olsson, Ola Olsson
Print a warning whenever an expected callback function
lacks implementation.
Signed-off-by: Ola Olsson <ola.olsson@sonymobile.com>
---
net/wireless/core.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/net/wireless/core.c b/net/wireless/core.c
index b091551..3a9c41b 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -352,6 +352,16 @@ struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv,
WARN_ON(ops->add_station && !ops->del_station);
WARN_ON(ops->add_mpath && !ops->del_mpath);
WARN_ON(ops->join_mesh && !ops->leave_mesh);
+ WARN_ON(ops->start_p2p_device && !ops->stop_p2p_device);
+ WARN_ON(ops->start_ap && !ops->stop_ap);
+ WARN_ON(ops->join_ocb && !ops->leave_ocb);
+ WARN_ON(ops->suspend && !ops->resume);
+ WARN_ON(ops->sched_scan_start && !ops->sched_scan_stop);
+ WARN_ON(ops->remain_on_channel && !ops->cancel_remain_on_channel);
+ WARN_ON(ops->tdls_channel_switch && !ops->tdls_cancel_channel_switch);
+ WARN_ON(ops->add_tx_ts && !ops->del_tx_ts);
+ WARN_ON(ops->set_tx_power && !ops->get_tx_power);
+ WARN_ON(ops->set_antenna && !ops->get_antenna);
alloc_size = sizeof(*rdev) + sizeof_priv;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] Print warnings for missing cfg80211_ops implementations
2015-12-16 21:43 [PATCH] Print warnings for missing cfg80211_ops implementations Ola Olsson
@ 2015-12-17 1:16 ` Joe Perches
[not found] ` <CABAco3BSaB9R6-nZQsmOnfXXy2ra-un0jjeBkyq4inzCybD5dw@mail.gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Joe Perches @ 2015-12-17 1:16 UTC (permalink / raw)
To: Ola Olsson, johannes; +Cc: linux-wireless, Ola Olsson
On Wed, 2015-12-16 at 22:43 +0100, Ola Olsson wrote:
> Print a warning whenever an expected callback function
> lacks implementation.
>
> Signed-off-by: Ola Olsson <ola.olsson@sonymobile.com>
> ---
> net/wireless/core.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/net/wireless/core.c b/net/wireless/core.c
> index b091551..3a9c41b 100644
> --- a/net/wireless/core.c
> +++ b/net/wireless/core.c
> @@ -352,6 +352,16 @@ struct wiphy *wiphy_new_nm(const struct cfg80211_ops *ops, int sizeof_priv,
> WARN_ON(ops->add_station && !ops->del_station);
> WARN_ON(ops->add_mpath && !ops->del_mpath);
> WARN_ON(ops->join_mesh && !ops->leave_mesh);
> + WARN_ON(ops->start_p2p_device && !ops->stop_p2p_device);
> + WARN_ON(ops->start_ap && !ops->stop_ap);
> + WARN_ON(ops->join_ocb && !ops->leave_ocb);
> + WARN_ON(ops->suspend && !ops->resume);
> + WARN_ON(ops->sched_scan_start && !ops->sched_scan_stop);
> + WARN_ON(ops->remain_on_channel && !ops->cancel_remain_on_channel);
> + WARN_ON(ops->tdls_channel_switch && !ops->tdls_cancel_channel_switch);
> + WARN_ON(ops->add_tx_ts && !ops->del_tx_ts);
> + WARN_ON(ops->set_tx_power && !ops->get_tx_power);
> + WARN_ON(ops->set_antenna && !ops->get_antenna);
maybe all of these should be a nand b?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Print warnings for missing cfg80211_ops implementations
[not found] ` <CABAco3BSaB9R6-nZQsmOnfXXy2ra-un0jjeBkyq4inzCybD5dw@mail.gmail.com>
@ 2015-12-17 5:19 ` Joe Perches
2015-12-17 7:34 ` Ola Olsson
0 siblings, 1 reply; 8+ messages in thread
From: Joe Perches @ 2015-12-17 5:19 UTC (permalink / raw)
To: Ola Olsson; +Cc: ola. olsson, linux-wireless, johannes
On Thu, 2015-12-17 at 05:19 +0100, Ola Olsson wrote:
> Hi Joe,
>
> > maybe all of these should be a nand b?
>
> You're right but i don't understand where the problem is. Please help
> me.
>
The code is currently like:
WARN_ON(ops->add_station && !ops->del_station);
but maybe it should be
WARN_ON((ops->add_station && !ops->del_station) ||
(!opt->add_station && ops->del_station))
etc...
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Print warnings for missing cfg80211_ops implementations
2015-12-17 5:19 ` Joe Perches
@ 2015-12-17 7:34 ` Ola Olsson
2015-12-17 7:57 ` Johannes Berg
0 siblings, 1 reply; 8+ messages in thread
From: Ola Olsson @ 2015-12-17 7:34 UTC (permalink / raw)
To: Joe Perches; +Cc: ola. olsson, linux-wireless, Johannes Berg
> but maybe it should be
>
> WARN_ON((ops->add_station && !ops->del_station) ||
> (!opt->add_station && ops->del_station))
>
> etc...
Ahh, got it! I really like your idea but I assume it's quite rare to
implement the "stop/del/leave/disconnect" callbacks and at the same
time forget to implement the "start/add/join/connect". You will
probably find out pretty quickly if the "start" functions are missing,
while it might take some time debugging why you lack the "stop"
functions (reinitialization issues/ resource leaks for example).
With that said, don't take my word for it, I was only following the
existing pattern and I assume someone else had a good reason in the
first place.
Best regards,
Ola
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Print warnings for missing cfg80211_ops implementations
2015-12-17 7:34 ` Ola Olsson
@ 2015-12-17 7:57 ` Johannes Berg
2015-12-17 11:01 ` Joe Perches
0 siblings, 1 reply; 8+ messages in thread
From: Johannes Berg @ 2015-12-17 7:57 UTC (permalink / raw)
To: Ola Olsson, Joe Perches; +Cc: ola. olsson, linux-wireless
On Thu, 2015-12-17 at 08:34 +0100, Ola Olsson wrote:
> > but maybe it should be
> >
> > WARN_ON((ops->add_station && !ops->del_station) ||
> > (!opt->add_station && ops->del_station))
> >
> > etc...
>
> Ahh, got it! I really like your idea but I assume it's quite rare to
> implement the "stop/del/leave/disconnect" callbacks and at the same
> time forget to implement the "start/add/join/connect". You will
> probably find out pretty quickly if the "start" functions are
> missing,
> while it might take some time debugging why you lack the "stop"
> functions (reinitialization issues/ resource leaks for example).
>
> With that said, don't take my word for it, I was only following the
> existing pattern and I assume someone else had a good reason in the
> first place.
>
Pretty much what you said :)
johannes
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Print warnings for missing cfg80211_ops implementations
2015-12-17 7:57 ` Johannes Berg
@ 2015-12-17 11:01 ` Joe Perches
2015-12-17 11:43 ` Ola Olsson
2015-12-17 12:43 ` Johannes Berg
0 siblings, 2 replies; 8+ messages in thread
From: Joe Perches @ 2015-12-17 11:01 UTC (permalink / raw)
To: Johannes Berg, Ola Olsson; +Cc: ola. olsson, linux-wireless
On Thu, 2015-12-17 at 08:57 +0100, Johannes Berg wrote:
> On Thu, 2015-12-17 at 08:34 +0100, Ola Olsson wrote:
> > > but maybe it should be
> > >
> > > WARN_ON((ops->add_station && !ops->del_station) ||
> > > (!opt->add_station && ops->del_station))
> > >
> > > etc...
> >
> > Ahh, got it! I really like your idea but I assume it's quite rare to
> > implement the "stop/del/leave/disconnect" callbacks and at the same
> > time forget to implement the "start/add/join/connect". You will
> > probably find out pretty quickly if the "start" functions are
> > missing,
> > while it might take some time debugging why you lack the "stop"
> > functions (reinitialization issues/ resource leaks for example).
> >
> > With that said, don't take my word for it, I was only following the
> > existing pattern and I assume someone else had a good reason in the
> > first place.
> >
>
> Pretty much what you said :)
Following patterns is good, I just think the
pattern could be trivially improved.
The test is a runtime check on what would ideally
be done at compile time.
Using
WARN_ON(!a ^ !b)
which is logically the same as what I wrote above
for clarity is simply a bit more coverage and maybe
even a bit run-time faster.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Print warnings for missing cfg80211_ops implementations
2015-12-17 11:01 ` Joe Perches
@ 2015-12-17 11:43 ` Ola Olsson
2015-12-17 12:43 ` Johannes Berg
1 sibling, 0 replies; 8+ messages in thread
From: Ola Olsson @ 2015-12-17 11:43 UTC (permalink / raw)
To: Joe Perches; +Cc: Johannes Berg, ola. olsson, linux-wireless
> Using
> WARN_ON(!a ^ !b)
> which is logically the same as what I wrote above
> for clarity is simply a bit more coverage and maybe
> even a bit run-time faster.
>
>
Didn't think about that. Love your bit fiddling trick! After an
approval/rejection of my patch I can submit another one with your
suggestion unless you want to do it yourself...
/Ola
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Print warnings for missing cfg80211_ops implementations
2015-12-17 11:01 ` Joe Perches
2015-12-17 11:43 ` Ola Olsson
@ 2015-12-17 12:43 ` Johannes Berg
1 sibling, 0 replies; 8+ messages in thread
From: Johannes Berg @ 2015-12-17 12:43 UTC (permalink / raw)
To: Joe Perches, Ola Olsson; +Cc: ola. olsson, linux-wireless
On Thu, 2015-12-17 at 03:01 -0800, Joe Perches wrote:
> Following patterns is good, I just think the
> pattern could be trivially improved.
It's a question of what makes sense though - nobody implements stop_xyz
without implementing start_xyz, and even if they do it's pointless.
It's just that if you have start_xyz most/all of your functional tests
might work, but we'd really like to have stop_xyz as well.
It's not *worse* to check for the XOR (like you suggest below), but
it's not really any better either.
> The test is a runtime check on what would ideally
> be done at compile time.
If you have any suggestions how to do that then that'd be great :) I
don't really see a way of doing that since this depends on the driver
and the driver might even fill the struct at runtime (like hwsim does
IIRC)
> Using
> WARN_ON(!a ^ !b)
> which is logically the same as what I wrote above
> for clarity is simply a bit more coverage and maybe
> even a bit run-time faster.
Don't think we have to worry much about the runtime overhead, but
that's a nice idea. As I said above though, I don't think it really
makes a difference.
johannes
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-12-17 12:43 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-16 21:43 [PATCH] Print warnings for missing cfg80211_ops implementations Ola Olsson
2015-12-17 1:16 ` Joe Perches
[not found] ` <CABAco3BSaB9R6-nZQsmOnfXXy2ra-un0jjeBkyq4inzCybD5dw@mail.gmail.com>
2015-12-17 5:19 ` Joe Perches
2015-12-17 7:34 ` Ola Olsson
2015-12-17 7:57 ` Johannes Berg
2015-12-17 11:01 ` Joe Perches
2015-12-17 11:43 ` Ola Olsson
2015-12-17 12:43 ` Johannes Berg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.