[PATCH] Print warnings for missing cfg80211_ops implementations

[PATCH] Print warnings for missing cfg80211_ops implementations

[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

Re: [PATCH] Print warnings for missing cfg80211_ops implementations

[Joe Perches]

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?

Re: [PATCH] Print warnings for missing cfg80211_ops implementations

[Joe Perches]

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...

Re: [PATCH] Print warnings for missing cfg80211_ops implementations

[Ola Olsson]

> 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

Re: [PATCH] Print warnings for missing cfg80211_ops implementations

[Johannes Berg]

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

Re: [PATCH] Print warnings for missing cfg80211_ops implementations

[Joe Perches]

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.

Re: [PATCH] Print warnings for missing cfg80211_ops implementations

[Ola Olsson]

> 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

Re: [PATCH] Print warnings for missing cfg80211_ops implementations

[Johannes Berg]

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